Move MemoryPressureObserverChromeOS instantiation from Chrome to Content

Move MemoryPressureObserverChromeOS instantiation from Chrome to Content

There are several proposed content CLs which are making use of the MemoryPressureObserverChromeOS::GetCurrentPressureLevel() call to adapt dynamically to the current memory pressure. Examples: The |OomPriorityManager|, the |RendererFrameManager|, ...

BUG=440556
TEST=-

Committed: https://crrev.com/5e3879cd0b54cb3d461dcbdcaa426429a7a7f123
Cr-Commit-Position: refs/heads/master@{#308459}

[Mr4D (OOO till 08-26), 2014-12-11 21:33:34]

skuhne@chromium.org changed reviewers:
+ danakj@chromium.org

[Mr4D (OOO till 08-26), 2014-12-12 17:19:53]

skuhne@chromium.org changed reviewers:
+ achuith@chromium.org
- danakj@chromium.org

[Mr4D (OOO till 08-26), 2014-12-12 17:20:22]

Achuith - can you please have a look?

Many thanks in advance!

[Mr4D (OOO till 08-26), 2014-12-12 17:58:49]

Never mind. This does not help either since chrome/browser is not accessible
from content/ and as such this way is useless as well.

Will need more thinking.

[achuithb, 2014-12-12 19:47:43]

On 2014/12/12 17:58:49, Mr4D wrote:
> Never mind. This does not help either since chrome/browser is not accessible
> from content/ and as such this way is useless as well.
> 
> Will need more thinking.

ok

[Mr4D (OOO till 08-26), 2014-12-12 20:41:52]

Okay, this should work now. Can you have a look!

Many thanks in advance!

[Mr4D (OOO till 08-26), 2014-12-15 16:58:16]

Ping. Thanks!

[achuithb, 2014-12-15 19:41:40]

lgtm

Sorry for the delay

[Mr4D (OOO till 08-26), 2014-12-15 19:42:31]

The CQ bit was checked by skuhne@chromium.org

[achuithb, 2014-12-15 19:42:36]

On 2014/12/15 19:41:40, achuithb wrote:
> lgtm
> 
> Sorry for the delay

Please update the description to say that you're moving
MemoryPressureObserverChromeOS (you spelt this wrong) from browser to content.

[commit-bot: I haz the power, 2014-12-15 19:43:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/800603002/40001

[commit-bot: I haz the power, 2014-12-15 20:09:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-15 20:09:31]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mr4D (OOO till 08-26), 2014-12-15 21:35:56]

skuhne@chromium.org changed reviewers:
+ creis@chromium.org

[Mr4D (OOO till 08-26), 2014-12-15 21:35:57]

Adding +creis for OWNERS review

[Charlie Reis, 2014-12-15 21:52:09]

> Move MemoryPressureObserverChromeOS instantiation to make it accessible

Accessible to what?  Please elaborate a bit in the CL description, since it's
not clear why this is needed.

https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.h:23: #if defined(OS_CHROMEOS)
This belongs with the OS-specific blocks below.

[Mr4D (OOO till 08-26), 2014-12-15 21:57:28]

Done. Please have another look!

[achuithb, 2014-12-15 21:57:29]

On 2014/12/15 21:52:09, Charlie Reis wrote:
> > Move MemoryPressureObserverChromeOS instantiation to make it accessible
> 
> Accessible to what?  Please elaborate a bit in the CL description, since it's
> not clear why this is needed.
> 

I did request that the description be updated. Not sure if you saw it, Stefan.


>
https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
> File content/browser/browser_main_loop.h (right):
> 
>
https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
> content/browser/browser_main_loop.h:23: #if defined(OS_CHROMEOS)
> This belongs with the OS-specific blocks below.

[Mr4D (OOO till 08-26), 2014-12-15 22:00:20]

Done!

[achuithb, 2014-12-15 22:05:18]

On 2014/12/15 22:00:20, Mr4D wrote:
> Done!

Not just the title, but the actual description text too please

[achuithb, 2014-12-15 22:05:21]

On 2014/12/15 22:00:20, Mr4D wrote:
> Done!

Not just the title, but the actual description text too please

[Charlie Reis, 2014-12-15 22:07:28]

On 2014/12/15 22:00:20, Mr4D wrote:
> Done!

It looks like you haven't uploaded a new patch set for my code comment?  Anyway,
thanks for updating the CL description.  A few nits.

> Move MemoryPressureObserverChromeOS instantiation from Browser to content to
make it accessible.

nit: Replace "Browser" with "chrome"
nit: drop "to make it accessible".  That's implicit from the sentence below, and
can be confused with Accessibility.


> There are several CL's queued which are making use of the
MemoryPressureObserverChromeOS::GetCurrentPressureLevel()
> call to adapt dynamically to the current memory pressure. Examples: The
|OomPriorityManager|, the |RendererFrameManager|, ...

nit: several proposed content CLs
(no apostrophe)

Please also make the CL subject match the first line of the description. 
Thanks!

[Mr4D (OOO till 08-26), 2014-12-15 22:12:11]

Okay - comment accordingly changed. Please have another look!

[Charlie Reis, 2014-12-15 22:18:53]

Thanks-- description looks good.  Did you see my comment in
content/browser/browser_main_loop.h?

[Mr4D (OOO till 08-26), 2014-12-15 22:21:46]

Please see comment - I think it is fine where it is.

https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.h:23: #if defined(OS_CHROMEOS)
Note: base::MemoryPressureObserverChromeOS.

The OS-specific block is in content::...

Or did you suggest to end the namespace there again and restart it once more?

[Charlie Reis, 2014-12-15 22:50:26]

LGTM

https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/800603002/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.h:23: #if defined(OS_CHROMEOS)
On 2014/12/15 22:21:46, Mr4D wrote:
> Note: base::MemoryPressureObserverChromeOS.
> 
> The OS-specific block is in content::...
> 
> Or did you suggest to end the namespace there again and restart it once more?

Ah, no, I missed the base:: part.  Apologies!

[Mr4D (OOO till 08-26), 2014-12-15 22:52:24]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-12-15 22:53:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/800603002/40001

[commit-bot: I haz the power, 2014-12-15 23:38:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-15 23:39:10]

Patchset 3 (id:??) landed as
https://crrev.com/5e3879cd0b54cb3d461dcbdcaa426429a7a7f123
Cr-Commit-Position: refs/heads/master@{#308459}