Make memory pressure IPC accessible from content/public.

Make memory pressure IPC accessible from content/public.

Code in chrome/browser will be responsible for deciding when and where to forward memory pressure notifications, so this API needs to be public.

BUG=483964

[chrisha, 2016-01-27 19:26:49]

The CQ bit was checked by chrisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-27 19:29:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1640423002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1640423002/1

[chrisha, 2016-01-27 20:30:33]

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

[chrisha, 2016-01-27 20:30:34]

creis: You looked at petrcermak@'s earlier related CLs. Care to take a look at
this?

[commit-bot: I haz the power, 2016-01-27 20:41:07]

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

[commit-bot: I haz the power, 2016-01-27 20:41:08]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-01-27 22:34:23]

On 2016/01/27 20:30:34, chrisha wrote:
> creis: You looked at petrcermak@'s earlier related CLs. Care to take a look at
> this?

Sure.  (Just curious-- what were the earlier CLs?  I don't remember seeing them,
and it looks like Nasko reviewed the memory_pressure_controller.h one.)

In general, we don't allow new things in content/public until they're used
outside content/.  Is it easy to include a use in this CL?

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
File content/public/browser/memory_pressure.h (right):

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:14: class MemoryMessageFilter;
Why is this needed?

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:18: //
content/browser/memory/memory_pressure_controller. See that header for more
nit: Add .h.

Also, there's no additional details in that header file.  Maybe there should be
more class level comments there?

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:21: CONTENT_EXPORT void
SendPressureNotification(
I think it's generally preferred to have these in a class rather than global
functions.

From http://www.chromium.org/developers/content-module/content-api:
"content/public should contain only interfaces, structs and (rarely) static
functions"

[chrisha, 2016-01-27 22:52:47]

> Sure.  (Just curious-- what were the earlier CLs?  I don't remember seeing
them,
> and it looks like Nasko reviewed the memory_pressure_controller.h one.)

I was going by memory... which apparently failed me :)

> In general, we don't allow new things in content/public until they're used
> outside content/.  Is it easy to include a use in this CL?

Use is coming in another CL. I thought it was easier to split the two. If you
want I can merge it with this one:

https://codereview.chromium.org/1641813002/

(I'll address the other comments separately.)

[Charlie Reis, 2016-01-27 22:56:55]

On 2016/01/27 22:52:47, chrisha wrote:
> > Sure.  (Just curious-- what were the earlier CLs?  I don't remember seeing
> them,
> > and it looks like Nasko reviewed the memory_pressure_controller.h one.)
> 
> I was going by memory... which apparently failed me :)

No worries.

> 
> > In general, we don't allow new things in content/public until they're used
> > outside content/.  Is it easy to include a use in this CL?
> 
> Use is coming in another CL. I thought it was easier to split the two. If you
> want I can merge it with this one:
> 
> https://codereview.chromium.org/1641813002/

Yes, that one's small enough that I'd prefer to have them merged.  That makes it
easier to find the justification for the API addition when digging through code
history.  Thanks!

> 
> (I'll address the other comments separately.)

[petrcermak, 2016-01-28 09:55:05]

petrcermak@chromium.org changed reviewers:
+ petrcermak@chromium.org

[petrcermak, 2016-01-28 09:55:05]

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
File content/public/browser/memory_pressure.cc (right):

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: Drop "(c)"
(https://www.chromium.org/developers/coding-style#TOC-File-headers)

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
File content/public/browser/memory_pressure.h (right):

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: Drop "(c)"
(https://www.chromium.org/developers/coding-style#TOC-File-headers)

[chrisha, 2016-01-28 21:22:14]

Responded to comments inline. Updated code will be at
https://codereview.chromium.org/1641813002/

Closing this CL.

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
File content/public/browser/memory_pressure.h (right):

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/01/28 09:55:05, petrcermak wrote:
> nit: Drop "(c)"
> (https://www.chromium.org/developers/coding-style#TOC-File-headers)

Done.

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:14: class MemoryMessageFilter;
On 2016/01/27 22:34:22, Charlie Reis wrote:
> Why is this needed?

Oops.. cut and paste error. Not needed, and removed.

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:18: //
content/browser/memory/memory_pressure_controller. See that header for more
On 2016/01/27 22:34:23, Charlie Reis wrote:
> nit: Add .h.
> 
> Also, there's no additional details in that header file.  Maybe there should
be
> more class level comments there?

Added more comments.

https://codereview.chromium.org/1640423002/diff/1/content/public/browser/memo...
content/public/browser/memory_pressure.h:21: CONTENT_EXPORT void
SendPressureNotification(
On 2016/01/27 22:34:22, Charlie Reis wrote:
> I think it's generally preferred to have these in a class rather than global
> functions.
> 
> From http://www.chromium.org/developers/content-module/content-api:
> "content/public should contain only interfaces, structs and (rarely) static
> functions"

I only mean to expose a few functions on MemoryPressureController. Should I
wholesale move MemoryPressureController to public/browser/memory, or should I
create a facade that only exposes the few functions external users care about?

[chrisha, 2016-01-28 21:26:12]

> I only mean to expose a few functions on MemoryPressureController. Should I
> wholesale move MemoryPressureController to public/browser/memory, or should I
> create a facade that only exposes the few functions external users care about?

Resuming our chat:

I'll rename the existing controller to MemoryPressureControllerImpl, and make
MemoryPressureController in public simple be a facade to the few functions
needing to be exposed.

[chrisha, 2016-01-28 21:50:14]

Description was changed from

==========
Make memory pressure IPC accessible from content/public.

Code in chrome/browser will be responsible for deciding when and where to
forward memory pressure notifications, so this API needs to be public.

BUG=483964
==========

to

==========
Make memory pressure IPC accessible from content/public.

Code in chrome/browser will be responsible for deciding when and where to
forward memory pressure notifications, so this API needs to be public.

BUG=483964
==========