Introduce MemoryCoordinatorProxy in base

Introduce MemoryCoordinatorProxy in base

This CL instroduces a new class MemoryCoordinatorProxy that is the proxy
of MemoryCoordinator in content/browser. MemoryCoordinator::
GetCurrentMemoryState to get the current memory state is now not
accessible by some MemoryCoordinatorClients, and MemoryCoordinatorProxy
offers the proxy function to access this.

This is used to get the current memory state by e.g. TabLoader in
chrome/browser or components in net.

BUG=639700
Committed: https://crrev.com/3524bd796cb8b8c57815dfa324e4cc66a74f5336
Cr-Commit-Position: refs/heads/master@{#427027}

[hajimehoshi, 2016-10-21 12:02:16]

hajimehoshi@chromium.org changed reviewers:
+ avi@chromium.org, bashi@google.com, chrisha@chromium.org, danakj@chromium.org,
haraken@chromium.org

[hajimehoshi, 2016-10-21 12:02:17]

PTAL

[hajimehoshi, 2016-10-21 12:06:28]

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

[commit-bot: I haz the power, 2016-10-21 12:06:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-10-21 13:01:18]

On 2016/10/21 12:02:17, hajimehoshi wrote:
> PTAL

So the content code lgtm, but the design of putting a proxy to something on a
higher layer into base/ is weird.

[commit-bot: I haz the power, 2016-10-21 13:37:20]

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

[commit-bot: I haz the power, 2016-10-21 13:37:20]

Dry run: This issue passed the CQ dry run.

[chrisha, 2016-10-21 18:41:09]

On 2016/10/21 13:01:18, Avi wrote:
> On 2016/10/21 12:02:17, hajimehoshi wrote:
> > PTAL
> 
> So the content code lgtm, but the design of putting a proxy to something on a
> higher layer into base/ is weird.

The initial intent was to keep things out of base. The code ended up content
(interface) and browser (implementation). When most of the way through porting
clients from MPL to MC we learned that there were indeed clients in net/, that
could only see base/. We lifted the minimal client interface from content to
base (MemoryCoordinatorClient and Registry). In pushing this further there is
need for clients to ask the coordinator for the current state. This is an
attempt to move a minimal interface to base for this purpose.

- The intent is is for there to be a minimal MC interface defined in base, as
well as the client interface.
- In content/browser/memory there's a broader MC interface, which is a superset
of that in base. The implementation is in browser/memory.
- In content/child/memory there's a broader MC interface, which is a superset of
that in base. The implemnentation is in content/child
- There are two MC concrete implementations, one for the browser (in browser/),
and the other for renderers (also in content/child/memory).

The general pattern of injection of implementation from a higher layer into an
interface defined in a lower layer is normal. Maybe the pattern is slightly
inverted. Would you prefer this to be an interface, which the two broader
interfaces in content/browser and content/child inherit from? At runtime, the
instance could then be injected into base?

Other suggestions?

[Avi (use Gerrit), 2016-10-21 18:45:23]

No...

Sigh. That sounds like a reasonable plan given the situation we're in. I... just
wish things wouldn't be so complicated.

LGTM

[danakj, 2016-10-21 18:56:30]

base LGTM with similar feelings.

https://codereview.chromium.org/2434353003/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2434353003/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1400:
base::Unretained(MemoryCoordinator::GetInstance())));
Can you leave a comment explaining why Unretained is safe here.

[bashi, 2016-10-24 00:10:24]

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

[bashi, 2016-10-24 00:10:25]

lgtm

https://codereview.chromium.org/2434353003/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl_unittest.cc (right):

https://codereview.chromium.org/2434353003/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl_unittest.cc:56:
std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
nit: Can we move out EnableForTesting() in memory_coordinator_browser_test.cc to
a separate file and use it here?

[hajimehoshi, 2016-10-24 06:51:06]

Thank you!

https://codereview.chromium.org/2434353003/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2434353003/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1400:
base::Unretained(MemoryCoordinator::GetInstance())));
On 2016/10/21 18:56:30, danakj wrote:
> Can you leave a comment explaining why Unretained is safe here.

Done.

https://codereview.chromium.org/2434353003/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl_unittest.cc (right):

https://codereview.chromium.org/2434353003/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl_unittest.cc:56:
std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
On 2016/10/24 00:10:25, bashi1 wrote:
> nit: Can we move out EnableForTesting() in memory_coordinator_browser_test.cc
to
> a separate file and use it here?

Done.

[hajimehoshi, 2016-10-24 06:53:06]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-10-24 06:53:07]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
danakj@chromium.org, bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/2434353003/#ps20001
(title: "Address on reviews")

[commit-bot: I haz the power, 2016-10-24 06:53:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 07:55:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-24 07:58:30]

Description was changed from

==========
Introduce MemoryCoordinatorProxy in base

This CL instroduces a new class MemoryCoordinatorProxy that is the proxy
of MemoryCoordinator in content/browser. MemoryCoordinator::
GetCurrentMemoryState to get the current memory state is now not
accessible by some MemoryCoordinatorClients, and MemoryCoordinatorProxy
offers the proxy function to access this.

This is used to get the current memory state by e.g. TabLoader in
chrome/browser or components in net.

BUG=639700
==========

to

==========
Introduce MemoryCoordinatorProxy in base

This CL instroduces a new class MemoryCoordinatorProxy that is the proxy
of MemoryCoordinator in content/browser. MemoryCoordinator::
GetCurrentMemoryState to get the current memory state is now not
accessible by some MemoryCoordinatorClients, and MemoryCoordinatorProxy
offers the proxy function to access this.

This is used to get the current memory state by e.g. TabLoader in
chrome/browser or components in net.

BUG=639700

Committed: https://crrev.com/3524bd796cb8b8c57815dfa324e4cc66a74f5336
Cr-Commit-Position: refs/heads/master@{#427027}
==========

[commit-bot: I haz the power, 2016-10-24 07:58:31]

Patchset 2 (id:??) landed as
https://crrev.com/3524bd796cb8b8c57815dfa324e4cc66a74f5336
Cr-Commit-Position: refs/heads/master@{#427027}

[haraken, 2016-10-24 09:01:16]

LGTM

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_proxy.cc (right):

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_proxy.cc:21: return MemoryState::NORMAL;

Is this an expected behavior?

I guess it's better to force all processes install the callback function, and
add DHECK(callback_).

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_proxy.h (right):

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_proxy.h:15: // The proxy of MemoryCoordinator to
be accessed from components that is not

is => are

[hajimehoshi, 2016-10-24 10:56:52]

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_proxy.cc (right):

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_proxy.cc:21: return MemoryState::NORMAL;
On 2016/10/24 09:01:15, haraken wrote:
> 
> Is this an expected behavior?
> 
> I guess it's better to force all processes install the callback function, and
> add DHECK(callback_).

Hmm, why we decided to use NORMAL as default is because the feature list might
not be initialized before a MemoryCoordinatorClient is created. This was needed
when the current memory state needed to be known at such client's constructor.
Now there are not actual such cases, but can we assume 
MemoryCoordinatorProxy::GetCurrentMemoryState is called AFTER the feature list
is initialized now?

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_proxy.h (right):

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_proxy.h:15: // The proxy of MemoryCoordinator to
be accessed from components that is not
On 2016/10/24 09:01:16, haraken wrote:
> 
> is => are

ouch

[haraken, 2016-10-24 11:35:39]

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_proxy.cc (right):

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_proxy.cc:21: return MemoryState::NORMAL;
On 2016/10/24 10:56:52, hajimehoshi wrote:
> On 2016/10/24 09:01:15, haraken wrote:
> > 
> > Is this an expected behavior?
> > 
> > I guess it's better to force all processes install the callback function,
and
> > add DHECK(callback_).
> 
> Hmm, why we decided to use NORMAL as default is because the feature list might
> not be initialized before a MemoryCoordinatorClient is created. This was
needed
> when the current memory state needed to be known at such client's constructor.
> Now there are not actual such cases, but can we assume 
> MemoryCoordinatorProxy::GetCurrentMemoryState is called AFTER the feature list
> is initialized now?

I think the feature list should be initialized at the very beginning of the
browser/renderer execution. Otherwise, it will confuse many things. Is it not
true?

[bashi, 2016-10-24 23:35:57]

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
File base/memory/memory_coordinator_proxy.cc (right):

https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
base/memory/memory_coordinator_proxy.cc:21: return MemoryState::NORMAL;
On 2016/10/24 11:35:39, haraken wrote:
> On 2016/10/24 10:56:52, hajimehoshi wrote:
> > On 2016/10/24 09:01:15, haraken wrote:
> > > 
> > > Is this an expected behavior?
> > > 
> > > I guess it's better to force all processes install the callback function,
> and
> > > add DHECK(callback_).
> > 
> > Hmm, why we decided to use NORMAL as default is because the feature list
might
> > not be initialized before a MemoryCoordinatorClient is created. This was
> needed
> > when the current memory state needed to be known at such client's
constructor.
> > Now there are not actual such cases, but can we assume 
> > MemoryCoordinatorProxy::GetCurrentMemoryState is called AFTER the feature
list
> > is initialized now?
> 
> I think the feature list should be initialized at the very beginning of the
> browser/renderer execution. Otherwise, it will confuse many things. Is it not
> true?
> 

It's probably true, but I think it's better to return the default state (NORMAL)
when callback_ is not set. Here are some reasons:
- It's hard to make sure that all callsites wait for memory coordinator
initialization. We can't initialize MC until base::FeatureList must be
initialized, but some MemoryCoordinatorClients (e.g. clients in cc/) are
instanciated before base::FeatureList is initialized.
- In tests we may not install a callback. base/ should work without depending
other components.

[haraken, 2016-10-25 05:07:50]

On 2016/10/24 23:35:57, bashi1 wrote:
>
https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
> File base/memory/memory_coordinator_proxy.cc (right):
> 
>
https://codereview.chromium.org/2434353003/diff/20001/base/memory/memory_coor...
> base/memory/memory_coordinator_proxy.cc:21: return MemoryState::NORMAL;
> On 2016/10/24 11:35:39, haraken wrote:
> > On 2016/10/24 10:56:52, hajimehoshi wrote:
> > > On 2016/10/24 09:01:15, haraken wrote:
> > > > 
> > > > Is this an expected behavior?
> > > > 
> > > > I guess it's better to force all processes install the callback
function,
> > and
> > > > add DHECK(callback_).
> > > 
> > > Hmm, why we decided to use NORMAL as default is because the feature list
> might
> > > not be initialized before a MemoryCoordinatorClient is created. This was
> > needed
> > > when the current memory state needed to be known at such client's
> constructor.
> > > Now there are not actual such cases, but can we assume 
> > > MemoryCoordinatorProxy::GetCurrentMemoryState is called AFTER the feature
> list
> > > is initialized now?
> > 
> > I think the feature list should be initialized at the very beginning of the
> > browser/renderer execution. Otherwise, it will confuse many things. Is it
not
> > true?
> > 
> 
> It's probably true, but I think it's better to return the default state
(NORMAL)
> when callback_ is not set. Here are some reasons:
> - It's hard to make sure that all callsites wait for memory coordinator
> initialization. We can't initialize MC until base::FeatureList must be
> initialized, but some MemoryCoordinatorClients (e.g. clients in cc/) are
> instanciated before base::FeatureList is initialized.
> - In tests we may not install a callback. base/ should work without depending
> other components.

Thanks, makes sense.