| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
if (!registry_) {Add `DLOG(WARNING)` might be appropriate here, to make developers aware that the registration silently failed. In particular, we would want to know this if a MemoryPressureListenerRegistrations are created in a new process type which doesn't instantiate the registry.
ObserverList<MemoryPressureListenerRegistryDestructionObserver>::UncheckedWhy is there a separate observer interface to observe destruction? Could OnBeforeMemoryPressureListenerRegistryDestroyed be a normal method on MemoryPressureListenerRegistration, invoked the same was as Notify?
Document that this is the registry used if this is a browser (i.e. not used with RunOtherNamedProcessTypeMain).
This is invoked by ContentMainRunnerImpl::Run(). Any reason not to emplace() the registry in ContentMainRunnerImpl::Run() before calling RunBrowser or RunOtherNamedProcessTypeMain, to avoid duplication of logic for the browser and non-browser cases?
base::MemoryPressureListenerRegistry memory_perssure_listener_registry_;```suggestion
base::MemoryPressureListenerRegistry memory_pressure_listener_registry_;
```
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Add `DLOG(WARNING)` might be appropriate here, to make developers aware that the registration silently failed. In particular, we would want to know this if a MemoryPressureListenerRegistrations are created in a new process type which doesn't instantiate the registry.
Good suggestion. The "there is an unsupported process type" case should not happen unless we create child process that doesn't go through chrome.exe, but it's definitely useful for debugging why a unit test wouldn't work, or if a MemoryPressureListenerRegistration is created too early or too late in the process' lifetime.
ObserverList<MemoryPressureListenerRegistryDestructionObserver>::UncheckedWhy is there a separate observer interface to observe destruction? Could OnBeforeMemoryPressureListenerRegistryDestroyed be a normal method on MemoryPressureListenerRegistration, invoked the same was as Notify?
Hmmm yeah your suggestion is obviously better. Thanks!
Document that this is the registry used if this is a browser (i.e. not used with RunOtherNamedProcessTypeMain).
Done
This is invoked by ContentMainRunnerImpl::Run(). Any reason not to emplace() the registry in ContentMainRunnerImpl::Run() before calling RunBrowser or RunOtherNamedProcessTypeMain, to avoid duplication of logic for the browser and non-browser cases?
Yeah it's a bit weird but ultimately, MemoryPressureListener will be deleted in favor of base::MemoryConsumer so I figured I'd create both registries side-by-side to ensure the behavior is exactly the same.
I added a TODO to clean this up in the future.
base::MemoryPressureListenerRegistry memory_perssure_listener_registry_;Patrick Monette```suggestion
base::MemoryPressureListenerRegistry memory_pressure_listener_registry_;
```
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Owners-Override | +1 |
LGTM
OO+1 for mechanical changes outside of //base
But let's ask for an owners review for //content/app
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
+a...@chromium.org for content/app/
The CL is mostly cleanup but the initialization stuff happens in content/app/content_main_runner_impl.
The initialization of MemoryPressureListenerRegistry intentionally mirrors the initialization of MemoryConsumerRegistry, which is why there is some repeat code that could technically be simplified, but MemoryPressureListenerRegistry will be deleted in the future anyways in favor of MemoryConsumerRegistry
PTAL
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
MemoryPressureListenerRegistry::MemoryPressureListenerRegistry() {Construction/destruction/NotifyMemoryPressure require external synchronization. Worth adding a THREAD_CHECKER?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Try it with threadchecker
Patrick MonetteYour latest patchset accidentally overwrote your commit message; please restore it.
Oops sorry about that. Now it's fixed.
MemoryPressureListenerRegistry::MemoryPressureListenerRegistry() {Construction/destruction/NotifyMemoryPressure require external synchronization. Worth adding a THREAD_CHECKER?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Owners-Override | +1 |
still LGTM / OO+1
But let's get an owners review for //content/app
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +2 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Explicitly instantiate the MemoryPressureListenerRegistry
A future change will add state to the MemoryPressureListenerRegistry,
allowing the notification of the current memory pressure level upon
registration of a listener.
If the registry is a static global that is never destroyed, then that
state leaks to other unit tests.
This CL changes the registry to require explicit instantiation. This
way, unit tests that actually need a registry can instantiate it
themselves, and the state does not leak to other unit tests.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |