Moves LowMemoryObserver into chromeos and renames

Moves LowMemoryObserver into chromeos and renames
LowMemoryListener. To break dependencies on chrome I made it have a
delegate rather than directly access the OomPriorityManager.

BUG=none
TEST=none
R=davemoore@chromium.org, jamescook@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198130

[James Cook, 2013-05-02 18:28:25]

Introduction of delegate and movement of code looks good. A couple questions.

https://codereview.chromium.org/14727005/diff/5002/chrome/chrome_browser_chro...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/14727005/diff/5002/chrome/chrome_browser_chro...
chrome/chrome_browser_chromeos.gypi:50:
'../chromeos/chromeos.gyp:chromeos_memory',
Why does this need to be its own module, with its own export / gypi / etc,
rather than part of the chromeos module?

(I suspect there's a reason, I just don't understand it. :-)

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
File chromeos/memory/low_memory_listener.cc (right):

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
chromeos/memory/low_memory_listener.cc:53: // |low_memory_callback| is run when
memory is low and
"and"....  I'm waiting in suspense here!

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
chromeos/memory/low_memory_listener.cc:166: weak_factory_(this) {
Maybe DCHECK that you are on UI thread?

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
chromeos/memory/low_memory_listener.cc:173: void LowMemoryListener::Start() {
How much do we usually sprinkle code with thread DCHECKs?  Do you think it's
obvious enough from the PostTask to FILE below that this function should be
called on UI?

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
File chromeos/memory/low_memory_listener.h (right):

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
chromeos/memory/low_memory_listener.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: 2013, since this is essentially a new file?

[sky, 2013-05-02 19:57:14]

New patch uploaded

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
File chromeos/memory/low_memory_listener.cc (right):

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
chromeos/memory/low_memory_listener.cc:53: // |low_memory_callback| is run when
memory is low and
On 2013/05/02 18:28:25, James Cook (Chromium) wrote:
> "and"....  I'm waiting in suspense here!

I removed the 'and' and added a '.':)

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
chromeos/memory/low_memory_listener.cc:166: weak_factory_(this) {
On 2013/05/02 18:28:25, James Cook (Chromium) wrote:
> Maybe DCHECK that you are on UI thread?

See comment below.

https://codereview.chromium.org/14727005/diff/5002/chromeos/memory/low_memory...
chromeos/memory/low_memory_listener.cc:173: void LowMemoryListener::Start() {
On 2013/05/02 18:28:25, James Cook (Chromium) wrote:
> How much do we usually sprinkle code with thread DCHECKs?  Do you think it's
> obvious enough from the PostTask to FILE below that this function should be
> called on UI?

I tend to only put them in places that could potentially be done on a wrong
thread or are not on the UI thread, such as OnMemoryLow below. Otherwise you
could argue we should have this check in every method, which feels like
overkill. Given this argument I think the DCHECK in OnMemoryLow below is the
only one that really makes sense.

[James Cook, 2013-05-02 20:11:21]

LGTM

[DaveMoore, 2013-05-02 21:06:43]

lgtm

[commit-bot: I haz the power, 2013-05-02 21:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/14727005/22001

[commit-bot: I haz the power, 2013-05-02 21:56:07]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-02 22:05:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/14727005/22001

[commit-bot: I haz the power, 2013-05-02 22:43:23]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-02 23:00:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/14727005/22001

[commit-bot: I haz the power, 2013-05-02 23:10:56]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-03 14:52:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/14727005/22001

[commit-bot: I haz the power, 2013-05-03 15:14:25]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[sky, 2013-05-03 16:41:16]

Committed patchset #6 manually as r198130 (presubmit successful).

[jam, 2014-03-31 16:02:08]

I just noticed this. chromeos sits below content, and so as a result it can't
include stuff from content. also, content includes chromeos, so we don't want
circular dependencies.

can  you use dependency injection to remove the BrowserThread dependency?

[James Cook, 2014-03-31 16:06:39]

I hate to suggest this, but what about just moving it back into Chrome?  The OOM
manager in production is tied to the concept of tabs.  I think this might be
left over from a desire to use the OOM system for blackwall.

[sky, 2014-03-31 16:07:32]

James is right. Feel free to move it back to chrome if you need to.

  -Scott

On Mon, Mar 31, 2014 at 9:06 AM,  <jamescook@chromium.org> wrote:
> I hate to suggest this, but what about just moving it back into Chrome?  The
> OOM
> manager in production is tied to the concept of tabs.  I think this might be
> left over from a desire to use the OOM system for blackwall.
>
>
> https://codereview.chromium.org/14727005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.