Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
815183002:
Using the new MemoryPressureListener instead of the LowMemoryObserver when the enhanced memory mana… (Closed)
|
Created:
6 years ago by Mr4D (OOO till 08-26) Modified:
5 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUsing the new MemoryPressureListener instead of the LowMemoryObserver when the enhanced memory management is turned on
This uses the new enhanced memory management - if the flag has turned it on. It will accelerate the tab discarding and use the |MemoryPressureListener| memory notification system.
BUG=440532
TEST=OomPriorityManagerUsingPressureListenerTest.OomPressureListener
Committed: https://crrev.com/cda33846b3070d2374853fa1c264ff134db847a0
Cr-Commit-Position: refs/heads/master@{#310524}
Patch Set 1 #
Total comments: 28
Patch Set 2 : Added unit test for memory_pressure_observer_chromeos and addressed #
Total comments: 24
Patch Set 3 : Solved addressability problem #
Total comments: 6
Patch Set 4 : Addressed #
Total comments: 10
Patch Set 5 : Addressed #Patch Set 6 : Fixed flaky unit test #Messages
Total messages: 41 (17 generated)
skuhne@chromium.org changed reviewers: + jamescook@chromium.org
This CL is not yet ready for submission since the question of where to get the MemoryPressureObserverChromeOS is still under investigation. However - since this is independent from that discussion I am sending it out for you to have a look at! Please have a look!
https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... File base/chromeos/memory_pressure_observer_chromeos.cc (right): https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:22: const int kModerateMemoryPressureCooldown = 10000 / kMemoryPressureIntervalInMS; nit: I would use two constants here, like "kModerateMemoryPressureCooldownMs = 10000" and then compute kModerateMemoryPressureCooldown. https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:93: base::Unretained(this))); Just to double-check: |this| always outlives the MessageLoop? If not, does this need a weakptr factory? https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:113: switch (current_memory_pressure_level_) { I find the logic here hard to understand. Maybe this would be better as a series of if()'s. In particular, I find it hard to think about whether this sends multiple notifications if the memory pressure stays in the moderate range (I don't think it does, but I was confused initially.) Maybe it would make more sense to break it into if (current == old) { // always notify for critical return; } // handle logic for changes https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:137: } I think this function is complex enough that it needs a little unit test. https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... File base/chromeos/memory_pressure_observer_chromeos.h (right): https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.h:62: int moderate_pressure_repeat_counter_; nit: maybe "moderate_pressure_repeat_count_"? https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... File chrome/browser/chromeos/memory/oom_priority_manager.cc (left): https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:410: // TODO(jamescook): Are there other things we could flush? Drive metadata? Hooray for working on this! https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... File chrome/browser/chromeos/memory/oom_priority_manager.cc (right): https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:109: base::MemoryPressureObserverChromeOS* memory_pressure_observer() { GetMemoryPressureObserver or similar https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:110: // TODO(skuhne): Before submitting this needs to move. Acknowledged. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:222: if (low_memory_observer_.get()) I might write a little function like "bool UseOldMemoryObserver()" or "bool UseNewMemoryListener()" to emphasize that there is an old way and a new way. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:223: low_memory_observer_->Start(); just return after this line https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:227: memory_pressure_listener_.reset(new base::MemoryPressureListener( optional: "using base::MemoryPressureListener;" might make some of this file easier to read https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:232: if (level == base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) Do you really need this? I think the manager is started pretty early in system startup, so it's unlikely you'll be under memory pressure already. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.cc:525: if (observer) Do you need this if()? https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.h:159: // The old observer for the kernel low memory signal. This is NULL if nit: NULL -> null in comments https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... File chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc (right): https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. no need to change this https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:32: virtual ~OomPriorityManagerUsingPressureListenerTest() {} nit: no virtual, just "override" https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:34: virtual void SetUpCommandLine(CommandLine* command_line) override { ditto https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:215: } nice test!
Addressed and test added. Still not landeable. But please have another look! https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... File base/chromeos/memory_pressure_observer_chromeos.cc (right): https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:22: const int kModerateMemoryPressureCooldown = 10000 / kMemoryPressureIntervalInMS; On 2014/12/19 19:38:07, James Cook wrote: > nit: I would use two constants here, like "kModerateMemoryPressureCooldownMs = > 10000" and then compute kModerateMemoryPressureCooldown. Done. https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:93: base::Unretained(this))); Originally it is/was not, but since this is in base it is a good idea to make it a weakptr factory to avoid any problems. Done. https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:113: switch (current_memory_pressure_level_) { Okay, Done. Note: The reason why I chose this however was that it a. shows better what is done in which level and b. it forces a change when we add another state back in. https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... File base/chromeos/memory_pressure_observer_chromeos.h (right): https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.h:62: int moderate_pressure_repeat_counter_; On 2014/12/19 19:38:07, James Cook wrote: > nit: maybe "moderate_pressure_repeat_count_"? Done. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager.h:159: // The old observer for the kernel low memory signal. This is NULL if On 2014/12/19 19:38:08, James Cook wrote: > nit: NULL -> null in comments Done. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... File chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc (right): https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. Sky asked for it, but I can also leave it this way. Done. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:32: virtual ~OomPriorityManagerUsingPressureListenerTest() {} On 2014/12/19 19:38:08, James Cook wrote: > nit: no virtual, just "override" Done. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:34: virtual void SetUpCommandLine(CommandLine* command_line) override { On 2014/12/19 19:38:08, James Cook wrote: > ditto Done. https://codereview.chromium.org/815183002/diff/1/chrome/browser/chromeos/memo... chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc:215: } Thanks!
skuhne@chromium.org changed reviewers: + piman@chromium.org
Please have a look! + Piman for owners review of contents/* Many thanks in advance! https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... File base/chromeos/memory_pressure_observer_chromeos.cc (right): https://codereview.chromium.org/815183002/diff/1/base/chromeos/memory_pressur... base/chromeos/memory_pressure_observer_chromeos.cc:137: } On 2014/12/19 19:38:07, James Cook wrote: > I think this function is complex enough that it needs a little unit test. Done.
https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos.cc (right): https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.cc:17: const int kMemoryPressureIntervalInMS = 1000; nit: I would rename to kMemoryPressureIntervalMs for consistency with below. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.cc:83: used_memory_in_percent_(GetUsedMemoryInPercent), Instead of doing this you could rename the function above to GetUsedMemoryInPercentImpl() and add a virtual function: virtual int GetUsedMemoryInPercent() { return GetUsedMemoryInPercentImpl(); } Then subclass this call in the test. Might be cleaner. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.cc:124: // In cas there is no memory pressure we do not notify. cas -> case https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos.h (right): https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:11: #include "base/memory/ref_counted.h" base/memory/weak_ptr.h ? https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:27: typedef int (*GetUsedMemoryInPercentCallback)(void); nit: using GetUsedMemeoryInPercentCallback = int (*)(); https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:43: void SetGetUsedMemoryInPercentCallbackForUnittest( nit: ForTest or ForTesting https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:47: friend MemoryPressureObserverChromeOSTest_CheckMemoryPressure_Test; Can you use FRIEND_TEST_ALL_PREFIXES for this? https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos_unittest.cc (right): https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos_unittest.cc:17: int s_memory_in_percent_override_ = 0; nit: I would name these like "g_memory_in_percent_override" or just "memory_in_percent_override". I don't think we have a coding style for these, but that's what I do. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos_unittest.cc:139: } nice test https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/memory/oom_priority_manager.cc (right): https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:222: if (low_memory_observer_.get()) Same comment as before. https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:232: if (level == base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) Same comment as before. https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:525: if (observer) I'm still curious if you need this if().
https://codereview.chromium.org/815183002/diff/40001/content/browser/browser_... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/815183002/diff/40001/content/browser/browser_... content/browser/browser_thread_impl.cc:486: base::MemoryPressureObserverChromeOS* GetMemoryPressureObserver() { This doesn't match the new method in the header, which is a static member of BrowserThread (this is a free function). https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... content/public/browser/browser_thread.h:160: #endif This doesn't really look like it belongs to BrowserThread. Should it be a top-level free function? Also, it would be nice if this was not a ChromeOS-only interface.
Please have another look! https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos.cc (right): https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.cc:17: const int kMemoryPressureIntervalInMS = 1000; On 2014/12/20 00:27:46, James Cook (OOO until Jan 5) wrote: > nit: I would rename to kMemoryPressureIntervalMs for consistency with below. Done. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.cc:83: used_memory_in_percent_(GetUsedMemoryInPercent), Oh my. I must have been blind. In deed! Done! https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.cc:124: // In cas there is no memory pressure we do not notify. On 2014/12/20 00:27:46, James Cook (OOO until Jan 5) wrote: > cas -> case Done. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos.h (right): https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:11: #include "base/memory/ref_counted.h" On 2014/12/20 00:27:47, James Cook (OOO until Jan 5) wrote: > base/memory/weak_ptr.h ? Done. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:27: typedef int (*GetUsedMemoryInPercentCallback)(void); On 2014/12/20 00:27:47, James Cook (OOO until Jan 5) wrote: > nit: using GetUsedMemeoryInPercentCallback = int (*)(); Done. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:43: void SetGetUsedMemoryInPercentCallbackForUnittest( On 2014/12/20 00:27:46, James Cook (OOO until Jan 5) wrote: > nit: ForTest or ForTesting Done. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:47: friend MemoryPressureObserverChromeOSTest_CheckMemoryPressure_Test; On 2014/12/20 00:27:47, James Cook (OOO until Jan 5) wrote: > Can you use FRIEND_TEST_ALL_PREFIXES for this? Done. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos_unittest.cc (right): https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos_unittest.cc:17: int s_memory_in_percent_override_ = 0; On 2014/12/20 00:27:47, James Cook (OOO until Jan 5) wrote: > nit: I would name these like "g_memory_in_percent_override" or just > "memory_in_percent_override". I don't think we have a coding style for these, > but that's what I do. Done. https://codereview.chromium.org/815183002/diff/20001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos_unittest.cc:139: } Thanks! https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/memory/oom_priority_manager.cc (right): https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:222: if (low_memory_observer_.get()) Was already done, but slipped into the next patchset. https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:232: if (level == base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) Since it was single line it was in compliance with the styleguide, but done. https://codereview.chromium.org/815183002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:525: if (observer) Absolutely! You will come here - even in stopped case. In that case there is neither a |LowMemoryObserver| nor a |MPOCOS|. https://codereview.chromium.org/815183002/diff/40001/content/browser/browser_... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/815183002/diff/40001/content/browser/browser_... content/browser/browser_thread_impl.cc:486: base::MemoryPressureObserverChromeOS* GetMemoryPressureObserver() { On 2014/12/20 02:53:15, piman (Very slow to review) wrote: > This doesn't match the new method in the header, which is a static member of > BrowserThread (this is a free function). Done. https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... content/public/browser/browser_thread.h:160: #endif Sure - we could also do it that way. Do you prefer top level free? After long consideration I thought that BrowserThread was a good fit. For the interface: That was what I initially intended. That was the reason for me to put it into a public (base) place. But since this could not be done for Android I was told to only do this for ChromeOS. Should we reconsider and do it for all OS'es?
https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... content/public/browser/browser_thread.h:160: #endif On 2014/12/20 03:01:05, Mr4D (OOO till Jan 6th) wrote: > Sure - we could also do it that way. Do you prefer top level free? I think I would prefer top-level free. This doesn't seem to fit with other members of this class. Also, it's not looking up a member of BrowserThreadImpl. So it really feels unrelated. See e.g. content::GetContextFactory > > After long consideration I thought that BrowserThread was a good fit. > > For the interface: > That was what I initially intended. That was the reason for me to put it into a > public (base) place. But since this could not be done for Android I was told to > only do this for ChromeOS. Should we reconsider and do it for all OS'es? I would prefer if we didn't have #ifdef warts in public APIs. In the future it's not inconceivable that we can support the memory pressure logic, or at least a subset, on other platforms.
On 2014/12/20 03:05:18, piman (Very slow to review) wrote: > https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... > File content/public/browser/browser_thread.h (right): > > https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... > content/public/browser/browser_thread.h:160: #endif > On 2014/12/20 03:01:05, Mr4D (OOO till Jan 6th) wrote: > > Sure - we could also do it that way. Do you prefer top level free? > > I think I would prefer top-level free. > This doesn't seem to fit with other members of this class. > Also, it's not looking up a member of BrowserThreadImpl. So it really feels > unrelated. > > See e.g. content::GetContextFactory > > > > > After long consideration I thought that BrowserThread was a good fit. > > > > For the interface: > > That was what I initially intended. That was the reason for me to put it into > a > > public (base) place. But since this could not be done for Android I was told > to > > only do this for ChromeOS. Should we reconsider and do it for all OS'es? > > I would prefer if we didn't have #ifdef warts in public APIs. In the future it's > not inconceivable that we can support the memory pressure logic, or at least a > subset, on other platforms. It's ok if we do that (a cross-platform API) in a follow-up though.
LGTM with a few nits https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos.h (right): https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:9: #include "base/gtest_prod_util.h" Are you using this? https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:41: protected: If you decide to FRIEND_TEST this could be private again. https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:56: // Get the memory pressure in percent. Note "virtual for testing" if that's the only reason it's virtual. https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos_unittest.cc (right): https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos_unittest.cc:161: } // namespace base nit: two spaces before // https://codereview.chromium.org/815183002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/memory/oom_priority_manager.cc (right): https://codereview.chromium.org/815183002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:108: base::MemoryPressureObserverChromeOS* memory_pressure_observer() { I would call this GetMemoryPressureObserver(). I don't know if we have a style guide for free functions, but it feels weird to me to use the inline_function_style for this.
Addressed. Piman, please have another look! https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... File content/public/browser/browser_thread.h (right): https://codereview.chromium.org/815183002/diff/40001/content/public/browser/b... content/public/browser/browser_thread.h:160: #endif Done that way. However - I still believe that this object should be a global base:: singleton accessible from everywhere - this object is a rather obvious thing which could be used from everywhere (even from base). Also - yes, I was trying to make this object OS unaware, but that change was rejected. If we ever want to make this uniform for all OS'es I am all for it. https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos.h (right): https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:9: #include "base/gtest_prod_util.h" With your requested change I need it again. :) https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:41: protected: On 2015/01/05 18:41:19, James Cook wrote: > If you decide to FRIEND_TEST this could be private again. Done. https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos.h:56: // Get the memory pressure in percent. On 2015/01/05 18:41:19, James Cook wrote: > Note "virtual for testing" if that's the only reason it's virtual. Done. https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... File base/chromeos/memory_pressure_observer_chromeos_unittest.cc (right): https://codereview.chromium.org/815183002/diff/60001/base/chromeos/memory_pre... base/chromeos/memory_pressure_observer_chromeos_unittest.cc:161: } // namespace base On 2015/01/05 18:41:19, James Cook wrote: > nit: two spaces before // Done. https://codereview.chromium.org/815183002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/memory/oom_priority_manager.cc (right): https://codereview.chromium.org/815183002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/memory/oom_priority_manager.cc:108: base::MemoryPressureObserverChromeOS* memory_pressure_observer() { On 2015/01/05 18:41:19, James Cook wrote: > I would call this GetMemoryPressureObserver(). I don't know if we have a style > guide for free functions, but it feels weird to me to use the > inline_function_style for this. Done.
LGTM for content/
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815183002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
skuhne@chromium.org changed reviewers: + thakis@chromium.org
Thakis, could you please do an OWNERS review for the base/BUILD files? Thanks!
base build files lgtm
The CQ bit was checked by skuhne@chromium.org
The CQ bit was unchecked by skuhne@chromium.org
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815183002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815183002/120001
The CQ bit was unchecked by skuhne@chromium.org
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815183002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by skuhne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/815183002/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cda33846b3070d2374853fa1c264ff134db847a0 Cr-Commit-Position: refs/heads/master@{#310524} |
