Use memory coordinator when --enable-memory-coordinator is specified (issue 2094583002 by bashi@chromium.org)

[ba...@chromium.org]

Reviewers: chrisha (slow), haraken
CL: https://codereview.chromium.org/2094583002/

Message:
PTAL

If this looks ok I'll ask OWNERS reviews.

Description:
Use memory coordinator when --enable-memory-coordinator is specified

This CL adds --enable-memory-coordinator flag which:
- makes the browser connects to a ChildMemoryCoordinator when a renderer is
launched
- disables memory pressure listener on renderers

This CL also adds a blink API for setting memory allocation mode.
A ChildMemoryCoordinator in a renderer calls the API based on memory events it
receives. The API is empty now and it will be implemented in follow-up CLs.

BUG=617492

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+357, -8 lines):
M components/memory_coordinator.gypi
M components/memory_coordinator/DEPS
A + components/memory_coordinator/browser/BUILD.gn
A components/memory_coordinator/browser/memory_state_notifier.h
A components/memory_coordinator/browser/memory_state_notifier.cc
M components/memory_coordinator/child/BUILD.gn
A components/memory_coordinator/child/blink_memory_coordinator_client.h
A components/memory_coordinator/child/blink_memory_coordinator_client.cc
M components/memory_coordinator/child/child_memory_coordinator_impl.h
M components/memory_coordinator/child/child_memory_coordinator_impl.cc
M components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
M content/browser/BUILD.gn
M content/browser/DEPS
M content/browser/browser_main_loop.h
M content/browser/browser_main_loop.cc
A content/browser/memory/memory_state_notifier_browsertest.cc
M content/browser/renderer_host/render_process_host_impl.cc
M content/content_browser.gypi
M content/content_renderer.gypi
M content/content_tests.gypi
M content/public/common/content_switches.h
M content/public/common/content_switches.cc
M content/renderer/BUILD.gn
M content/renderer/DEPS
M content/renderer/render_thread_impl.cc
M content/test/BUILD.gn
M third_party/WebKit/Source/web/WebMemoryCoordinator.cpp
M third_party/WebKit/public/web/WebMemoryCoordinator.h

[ba...@chromium.org]

ping?

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/browser/memory_state_notifier.cc
File components/memory_coordinator/browser/memory_state_notifier.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/browser/memory_state_notifier.cc#newcode29
components/memory_coordinator/browser/memory_state_notifier.cc:29: int
id = GetNextId();

Would you help me understand why you need an id system? (i.e., why it's
not enough to just keep the list of child coordinators)

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc
File
components/memory_coordinator/child/blink_memory_coordinator_client.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc#newcode24
components/memory_coordinator/child/blink_memory_coordinator_client.cc:24:
blink::WebMemoryCoordinator::setAllocationMode(

As commented in the other place, I want to make the Blink API consistent
with MemoryCoordinatorClient's API. So I'd prefer just redirecting the
OnMemoryStateChanged API to Blink instead of translating it to
setAllocationMode or something.

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/child_memory_coordinator_impl.cc
File
components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode15
components/memory_coordinator/child/child_memory_coordinator_impl.cc:15:
new ChildMemoryCoordinatorImpl(std::move(request), clients);

Nit: new ChildMemoryCoordinatorImpl(std::move(request), (new ClientList)

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode23
components/memory_coordinator/child/child_memory_coordinator_impl.cc:23:
blink_client_(new BlinkMemoryCoordinatorClient) {

Do we need the member variable |blink_client_|?

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom#newcode24
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24:
GetLastState() => (MemoryState state);

GetLastState => GetCurrentState ?

https://codereview.chromium.org/2094583002/diff/20001/third_party/WebKit/public/web/WebMemoryCoordinator.h
File third_party/WebKit/public/web/WebMemoryCoordinator.h (right):

https://codereview.chromium.org/2094583002/diff/20001/third_party/WebKit/public/web/WebMemoryCoordinator.h#newcode24
third_party/WebKit/public/web/WebMemoryCoordinator.h:24: BLINK_EXPORT
static void setAllocationMode(WebMemoryAllocationMode);

Would it make more sense to introduce onMemoryStateChanged() API instead
of setAllocationMode? I want the Blink API to be more consistent with
the Chromium-side memory-coordinator API.

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/browser/memory_state_notifier.cc
File components/memory_coordinator/browser/memory_state_notifier.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/browser/memory_state_notifier.cc#newcode29
components/memory_coordinator/browser/memory_state_notifier.cc:29: int
id = GetNextId();

On 2016/06/27 02:17:07, haraken wrote:
>
> Would you help me understand why you need an id system? (i.e., why
it's not
> enough to just keep the list of child coordinators)

ChildMemoryCoordinatorPtr is an alias for InterfacePtr and it doesn't
support copy and compare, so
- we can't use std::set
- when we use std::vector we need to pass the pointer of the
InterfacePtr to RemoveChild() and RemoveChild() needs to iterate the
list to remove it from the list. The same can be said to other list-like
containers

I think using IDs is simple.

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc
File
components/memory_coordinator/child/blink_memory_coordinator_client.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc#newcode24
components/memory_coordinator/child/blink_memory_coordinator_client.cc:24:
blink::WebMemoryCoordinator::setAllocationMode(

On 2016/06/27 02:17:07, haraken wrote:
>
> As commented in the other place, I want to make the Blink API
consistent with
> MemoryCoordinatorClient's API. So I'd prefer just redirecting the
> OnMemoryStateChanged API to Blink instead of translating it to
setAllocationMode
> or something.
>
>

That requires duplicate enums in public/web which I don't want to
introduce. We'll end up translating the enum to actual actions and I
think this is an appropriate boundary.

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/child_memory_coordinator_impl.cc
File
components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode15
components/memory_coordinator/child/child_memory_coordinator_impl.cc:15:
new ChildMemoryCoordinatorImpl(std::move(request), clients);

On 2016/06/27 02:17:07, haraken wrote:
>
> Nit: new ChildMemoryCoordinatorImpl(std::move(request), (new
ClientList)

Done.

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode23
components/memory_coordinator/child/child_memory_coordinator_impl.cc:23:
blink_client_(new BlinkMemoryCoordinatorClient) {

On 2016/06/27 02:17:07, haraken wrote:
>
> Do we need the member variable |blink_client_|?

Someone needs to have the ownership of BlinkMemoryCoordinatorClient
because ClientList doesn't take the ownership. I think
ChildMemoryCoordinatorImpl is a good place.

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom#newcode24
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24:
GetLastState() => (MemoryState state);

On 2016/06/27 02:17:07, haraken wrote:
>
> GetLastState => GetCurrentState ?

Done.

https://codereview.chromium.org/2094583002/diff/20001/content/public/common/content_switches.cc
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2094583002/diff/20001/content/public/common/content_switches.cc#newcode410
content/public/common/content_switches.cc:410: const char
kEnableMemoryCoordinator[] = "enable-memory-coordinator";
I'll switch to use base::FeatureList

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc
File
components/memory_coordinator/child/blink_memory_coordinator_client.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc#newcode24
components/memory_coordinator/child/blink_memory_coordinator_client.cc:24:
blink::WebMemoryCoordinator::setAllocationMode(

On 2016/06/27 03:49:29, bashi1 wrote:
> On 2016/06/27 02:17:07, haraken wrote:
> >
> > As commented in the other place, I want to make the Blink API
consistent with
> > MemoryCoordinatorClient's API. So I'd prefer just redirecting the
> > OnMemoryStateChanged API to Blink instead of translating it to
> setAllocationMode
> > or something.
> >
> >
>
> That requires duplicate enums in public/web which I don't want to
introduce.
> We'll end up translating the enum to actual actions and I think this
is an
> appropriate boundary.

But you're already introducing WebMemoryAllocationMode::Normal etc? I
don't think we can avoid duplicating the enum.

A couple of questions:

- Are you going to add a purging mode to setAllocationMode?

- Are you going to add setAllocationMode to all clients (cc, skia, v8
etc)?

- What's a difference between "onMemoryStateChanged" and
"setAllocationMode"?

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc
File
components/memory_coordinator/child/blink_memory_coordinator_client.cc
(right):

https://codereview.chromium.org/2094583002/diff/20001/components/memory_coordinator/child/blink_memory_coordinator_client.cc#newcode24
components/memory_coordinator/child/blink_memory_coordinator_client.cc:24:
blink::WebMemoryCoordinator::setAllocationMode(

On 2016/06/27 04:48:33, haraken wrote:
> On 2016/06/27 03:49:29, bashi1 wrote:
> > On 2016/06/27 02:17:07, haraken wrote:
> > >
> > > As commented in the other place, I want to make the Blink API
consistent
> with
> > > MemoryCoordinatorClient's API. So I'd prefer just redirecting the
> > > OnMemoryStateChanged API to Blink instead of translating it to
> > setAllocationMode
> > > or something.
> > >
> > >
> >
> > That requires duplicate enums in public/web which I don't want to
introduce.
> > We'll end up translating the enum to actual actions and I think this
is an
> > appropriate boundary.
>
> But you're already introducing WebMemoryAllocationMode::Normal etc? I
don't
> think we can avoid duplicating the enum.

It's not going to be the same as mojom::MemoryState. I don't plan to add
Purge enum. Instead, I'll add purge() (or similar) API in
WebMemoryCoordinator.

>
> A couple of questions:
>
> - Are you going to add a purging mode to setAllocationMode?

No, as described above.

>
> - Are you going to add setAllocationMode to all clients (cc, skia, v8
etc)?

No. I may do similar things but if ethese components already provide
APIs for purging/throttling I'll just use it.

>
> - What's a difference between "onMemoryStateChanged" and
"setAllocationMode"?

The former is notification, the later is action.

At some point we need to have swich or if statements to convert
notification to action, and I think mc -> blink boundary would be a good
place to do that.

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

I was assuming that we would do the conversion in each allocator component. If
we do that, will it mess up many allocators?

If it will, I'm okay with doing the conversion from the notification to the
actions, but then I want to make the APIs for the actions consistent throughout
all the allocators (i.e., add setAllocationMode to cc, skia, v8 etc).

My main point is that I want to make allocator-facing APIs consistent.



https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

Allocator is ambiguous in this context. If allocator == components, we already
have MemoryCoordinatorClient for consistency. What the actual meaning of
allocator? And what's the benefit of having the consistent APIs even we already
have MemoryCoordinatorClient?

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

I meant "a component that implements logic to throttle/unthrottle/suspend its
memory" by allocator. It includes PartitionAlloc, FontCache etc.

I'd ask the question in the other way. What's the benefit of introducing
different APIs for those allocators? As far as I understand, one of the goals of
MemoryCoordinator is that the API is actionable. If the API is just a
notification and not actionable, I'd say that it's a bug of the API. (i.e., my
preference is to design the API so that everything can just use the API.)


https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

I'm afraid that this would mess up code base. We'll add a lot of clients which
only have one line (e.g. invalidate() for FontCacheMemoryCoordiantorClient) for
purge/throttle/whatever if we take this approach. I'm thinking one client for
Blink, specifically core/dom/MemoryCoordinator.

>
> I'd ask the question in the other way. What's the benefit of introducing
> different APIs for those allocators? As far as I understand, one of the goals
of
> MemoryCoordinator is that the API is actionable. If the API is just a
> notification and not actionable, I'd say that it's a bug of the API. (i.e., my
> preference is to design the API so that everything can just use the API.)

No redundant abstraction. I don't think that ChildMemoryCoordinator need such
granularity (awareness of FontCache, PartitionAlloc). CMC can just ask
BlinkMemoryCoordinator to purge/throttle/whatever defined in
MemoryCoordinatorClient. That's the memory coordinator API in my mind. Could you
elaborate pros of your approach?

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

For consistency. It would be easier to understand if all allocators provide
methods for purge/throttle/whatever than they provide random methods for
controlling their memory.

If you really want to split the notification API from the action API, I won't
object to the idea. But even in that case, I want to make the action API
consistent.


https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

My proposal doesn't introduce any inconsistency. Let me clarify what kind of
clients we will have in renderers (in my proposal):

v8, cc, skia, blink

These will have the same API. I guess that in your mind, client would be:

v8, cc, skia, partition_alloc, oilpan, font_cache, memory_cache, ...

This doesn't seem to have the same levels of abstraction, and will introduce
unnecessary abstraction (Adding
{FontCache,MemoryCache,Oilpan,PartitionAlloc}MemoryCoordinator instead of one
BlinkMemoryCoordinator)

[har...@chromium.org]

I'm not saying that we should add the MC abstraction to all the clients. I just
want to keep the API shape as consistent as possible. What's a problem of
consistently using the onMemoryStateChanged() API for all the clients (which
don't need to inherit from MC)?

If you think that the onMemoryStateChanged() API is not convenient, I'd say we
should revise the API.


https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

Discussed offline. I'll drop blink related code from this CL. For
BlinkMemoryCoordinator I'll create a separate CL when the design is stabilized.

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

Dropped BlinkMemoryCoordinator.

chrisha@: any chance to take a look?

https://codereview.chromium.org/2094583002/diff/20001/content/public/common/content_switches.cc
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2094583002/diff/20001/content/public/common/content_switches.cc#newcode410
content/public/common/content_switches.cc:410: const char
kEnableMemoryCoordinator[] = "enable-memory-coordinator";

On 2016/06/27 03:49:29, bashi1 wrote:

> I'll switch to use base::FeatureList

Done.

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

non-owner LGTM



https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.cc
File components/memory_coordinator/browser/memory_state_notifier.cc
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.cc#newcode31
components/memory_coordinator/browser/memory_state_notifier.cc:31:
&MemoryStateNotifier::RemoveChild, base::Unretained(this), id));

I'm not sure if the ID system is the best way to implement this. I'll
defer the review to Chromium's experts.

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/common/memory_coordinator_features.cc
File components/memory_coordinator/common/memory_coordinator_features.cc
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/common/memory_coordinator_features.cc#newcode1
components/memory_coordinator/common/memory_coordinator_features.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.

2016

https://codereview.chromium.org/2094583002/

[chr...@chromium.org]

I don't think the MemoryStateNotifier is what you want or need.

It grabs memory pressure notifications from the browser process and sends them
all to all of the child processes. Right now messages are forwarded via logic in
TabManager, and send using code in the various MemoryMessageFilter classes.

For now you're interested in having messages arrive in a child memory
coordinator. The easiest way to do that is to not modify anything at all in the
browser. Then, using your Notifier callback mechanism, intercept memory pressure
notifications coming in to renderer processes. Then, translate these to
MemoryCoordinator "pressure levels", and forward them to
MemoryCoordinatorClients in via the ChildProcessMemoryCoordinator.

You shouldn't need to change any logic in the browser initially. We can then in
parallel lift the PriorityTracker out of the TabManager, create the main browser
MemoryCoordinator, create our own parallel messaging mechanism in the various
MemoryMessageFilter classes (necessary so we can enable/disable/override this
functionality). Then we can remove the MemoryNotifier mechanism entirely.

Do you have a detailed roadmap I can look at? Would be easiest to coordinate on
something like that initially.

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.cc
File components/memory_coordinator/browser/memory_state_notifier.cc
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.cc#newcode31
components/memory_coordinator/browser/memory_state_notifier.cc:31:
&MemoryStateNotifier::RemoveChild, base::Unretained(this), id));

On 2016/06/27 11:16:00, haraken wrote:
>
> I'm not sure if the ID system is the best way to implement this. I'll
defer the
> review to Chromium's experts.

I don't see any problem with this, but you could also just use a
std::set and pass around the raw pointer to the child?

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.h
File components/memory_coordinator/browser/memory_state_notifier.h
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.h#newcode38
components/memory_coordinator/browser/memory_state_notifier.h:38: void
RemoveChild(int id);
Documentation for these functions would be helpful for somebody reading
the code. (ie. RemoveChild is used as an on-error callback to remove a
child when the connection dies.)

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/child/child_memory_coordinator_impl.cc
File
components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode20
components/memory_coordinator/child/child_memory_coordinator_impl.cc:20:
// TODO(bashi): Add clients (e.g. skia).
Wouldn't it make more sense for clients to add themselves? ie. via a
mechanism in MemoryCoordinatorClient constructor? (Something more akin
to MemoryPressureListener's mechanism?)

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode31
components/memory_coordinator/child/child_memory_coordinator_impl.cc:31:
void ChildMemoryCoordinatorImpl::GetCurrentState(
Do we need this call? The state is centrally imposed from the
coordinator in the browser. It shouldn't need to query the individual
child process for its memory state.

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

On 2016/06/27 20:35:30, chrisha (slow) wrote:
> I don't think the MemoryStateNotifier is what you want or need.
>
> It grabs memory pressure notifications from the browser process and sends them
> all to all of the child processes. Right now messages are forwarded via logic
in
> TabManager, and send using code in the various MemoryMessageFilter classes.
>
> For now you're interested in having messages arrive in a child memory
> coordinator. The easiest way to do that is to not modify anything at all in
the
> browser. Then, using your Notifier callback mechanism, intercept memory
pressure
> notifications coming in to renderer processes. Then, translate these to
> MemoryCoordinator "pressure levels", and forward them to
> MemoryCoordinatorClients in via the ChildProcessMemoryCoordinator.
>
> You shouldn't need to change any logic in the browser initially. We can then
in
> parallel lift the PriorityTracker out of the TabManager, create the main
browser
> MemoryCoordinator, create our own parallel messaging mechanism in the various
> MemoryMessageFilter classes (necessary so we can enable/disable/override this
> functionality). Then we can remove the MemoryNotifier mechanism entirely.

Hmm, what I'm trying to do in V0 are:
- MemoryMessage IPC is replaced with Mojo
- All memory coordinator messages are routed to the browser process
- browser -> browser
- browser -> renderer
- renderer -> browser -> renderer
- Migrate MemoryPressureListener instances to MemoryCoordinatorClient
- This includes browser side
- Remove MemoryPressureListener

It seems you are suggesting the last two things in V0. I'm not sure only doing
the last two things is worth doing separately because it won't provide any
structual differences. One of the key goal of memory coordinator is to have a
consistent notification mechanism and I think it includes how to communicate
between browser and renderers.

>
> Do you have a detailed roadmap I can look at? Would be easiest to coordinate
on
> something like that initially.

The doc I sent previously partially described a roadmap (I've been revising it).
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVIYI/edit#

This doc isn't enough to answer your questions. I'll add more explanations in
the doc.

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.cc
File components/memory_coordinator/browser/memory_state_notifier.cc
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/browser/memory_state_notifier.cc#newcode31
components/memory_coordinator/browser/memory_state_notifier.cc:31:
&MemoryStateNotifier::RemoveChild, base::Unretained(this), id));

On 2016/06/27 20:35:30, chrisha (slow) wrote:
> On 2016/06/27 11:16:00, haraken wrote:
> >
> > I'm not sure if the ID system is the best way to implement this.
I'll defer
> the
> > review to Chromium's experts.
>
> I don't see any problem with this, but you could also just use a
std::set and
> pass around the raw pointer to the child?

Yes. I'd just prefer using IDs but raw pointers should work.

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/child/child_memory_coordinator_impl.cc
File
components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode20
components/memory_coordinator/child/child_memory_coordinator_impl.cc:20:
// TODO(bashi): Add clients (e.g. skia).

On 2016/06/27 20:35:30, chrisha (slow) wrote:
> Wouldn't it make more sense for clients to add themselves? ie. via a
mechanism
> in MemoryCoordinatorClient constructor? (Something more akin to
> MemoryPressureListener's mechanism?)

It was my first attempt. MemoryPressureListener uses a singleton to
add/remove listeners. On the other hand ChildMemoryCoordinator is a mojo
interface impl and I couldn't figure how to access it without
InterfaceRequest.

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode31
components/memory_coordinator/child/child_memory_coordinator_impl.cc:31:
void ChildMemoryCoordinatorImpl::GetCurrentState(

On 2016/06/27 20:35:30, chrisha (slow) wrote:
> Do we need this call? The state is centrally imposed from the
coordinator in the
> browser. It shouldn't need to query the individual child process for
its memory
> state.

This is for testing. Without it it's hard to make CL smaller. We may
want to rename it as GetCurrentStateForTesting().

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/common/memory_coordinator_features.cc
File components/memory_coordinator/common/memory_coordinator_features.cc
(right):

https://codereview.chromium.org/2094583002/diff/60001/components/memory_coordinator/common/memory_coordinator_features.cc#newcode1
components/memory_coordinator/common/memory_coordinator_features.cc:1:
// Copyright 2014 The Chromium Authors. All rights reserved.

On 2016/06/27 11:16:00, haraken wrote:
>

> 2016

Acknowledged.

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

On 2016/06/27 22:40:23, bashi1 wrote:
> The doc I sent previously partially described a roadmap (I've been revising
it).
>
https://docs.google.com/document/d/1rGeUzhfzIKwmBzUcqUEwxEpLQQRq3PulRuHwFBlVIYI/edit
>

> This doc isn't enough to answer your questions. I'll add more explanations in
> the doc.

Updated.

https://codereview.chromium.org/2094583002/

[chr...@chromium.org]

My problem with this CL is the dispatch unconditionally from browser to all
child processes. The point is for the browser to decide (inside of Memory
Coordinator) which levels to apply to which renderers, in response to global
memory pressure; not to blanket all renderers with the same notification.

For now that logic is in TabManager [1], which should be reworked to use a mojo
notification if memory coordinator is enabled (just need to replace the
callback).

[1]
https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?q=tab_manager.cc&sq=package:chromium&l=770

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

Hmm, I thought that it's to be done by tab manager -> priority tracker work.
What in my mind was just creating basic notification mechanism for memory
coordinator.

I'll take closer look at tab manager then.

https://codereview.chromium.org/2094583002/

[Chris Hamilton]

It will be done by the tab manager refactor, but I'd be uncomfortable turning things on this way in the meantime; it would be very heavy handed. If this is not intended to be enabled for users at all, but only internally on the way to launching the feature, then I'm fine with the notify-all-children, provided a giant warning comment with a TODO.

Otherwise, I'd prefer piggybacking in the slightly broken but much more conservative existing notification mechanism.

[ba...@chromium.org]

On 2016/06/30 03:17:15, chrisha (slow) wrote:
> It will be done by the tab manager refactor, but I'd be uncomfortable
> turning things on this way in the meantime; it would be very heavy handed.
> If this is not intended to be enabled for users at all, but only internally
> on the way to launching the feature, then I'm fine with the
> notify-all-children, provided a giant warning comment with a TODO.
>
> Otherwise, I'd prefer piggybacking in the slightly broken but much more
> conservative existing notification mechanism.

I haven't took a look at tab manager yet (as I was ooo yesterday) but let me
give it a try. I'm revising this CL with some structural changes and it would
take couple of hours.

>
> On Wed, Jun 29, 2016, 20:09 <mailto:ba...@chromium.org> wrote:
>
> > On 2016/06/29 21:15:31, chrisha (slow) wrote:
> > > My problem with this CL is the dispatch unconditionally from browser to
> > all
> > > child processes. The point is for the browser to decide (inside of Memory
> > > Coordinator) which levels to apply to which renderers, in response to
> > global
> > > memory pressure; not to blanket all renderers with the same notification.
> > >
> > > For now that logic is in TabManager [1], which should be reworked to use
> > a
> > mojo
> > > notification if memory coordinator is enabled (just need to replace the
> > > callback).
> > >
> > > [1]
> > >
> >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?q=tab_manager.cc&sq=package:chromium&l=770
> >
> > Hmm, I thought that it's to be done by tab manager -> priority tracker
> > work.
> > What in my mind was just creating basic notification mechanism for memory
> > coordinator.
> >
> > I'll take closer look at tab manager then.
> >
> > https://codereview.chromium.org/2094583002/
> >
>

> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-revie...@chromium.org.



https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

Revised. Chris, WDYT?

I made some structural changes:
- Added MemoryCoordinator mojo interface to align changes in the V0 design doc.
- ChildMemoryCoordinator is owned by RenderThreadImpl instead of self-deleting
by StrongBinding. This enables us to add clients from anywhere in content/ by
RenderThreadImpl::current()->memory_coordinator()->RegisterClient().

As for dispatching signals to renderers, this CL doesn't actually replace
MemoryPressureListener so I removed notifier. The browser tests added in this CL
just checks whether MC <-> CMC communication works. I'll create a separate CL
for tab manager and memory coordinator integration once this is settled.

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

chrisha@: friendly ping :)

https://codereview.chromium.org/2094583002/

[chr...@chromium.org]

A few more questions...


https://codereview.chromium.org/2094583002/diff/120001/components/memory_coordinator/child/child_memory_coordinator_impl.h
File components/memory_coordinator/child/child_memory_coordinator_impl.h
(right):

https://codereview.chromium.org/2094583002/diff/120001/components/memory_coordinator/child/child_memory_coordinator_impl.h#newcode28
components/memory_coordinator/child/child_memory_coordinator_impl.h:28:
// Reigsters/unregisters a client. Does not take ownership of client.
Registers*

https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory/memory_coordinator_impl_browsertest.cc
File content/browser/memory/memory_coordinator_impl_browsertest.cc
(right):

https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory/memory_coordinator_impl_browsertest.cc#newcode26
content/browser/memory/memory_coordinator_impl_browsertest.cc:26:
[&state](memory_coordinator::mojom::ChildMemoryCoordinator* child) {
Capturing state by ref here, and by value below? Be consistent?

https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory/memory_coordinator_impl_browsertest.cc#newcode51
content/browser/memory/memory_coordinator_impl_browsertest.cc:51:
child->GetCurrentState(
GetCurrentState is only used for testing, no? Maybe better off named
GetCurrentStateForTesting?

Also, why the need to pass in a callback? Can't it be a simple getter
function?

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/120001/components/memory_coordinator/child/child_memory_coordinator_impl.h
File components/memory_coordinator/child/child_memory_coordinator_impl.h
(right):

https://codereview.chromium.org/2094583002/diff/120001/components/memory_coordinator/child/child_memory_coordinator_impl.h#newcode28
components/memory_coordinator/child/child_memory_coordinator_impl.h:28:
// Reigsters/unregisters a client. Does not take ownership of client.

On 2016/07/07 02:53:43, chrisha (slow) wrote:
> Registers*

Done.

https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory/memory_coordinator_impl_browsertest.cc
File content/browser/memory/memory_coordinator_impl_browsertest.cc
(right):

https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory/memory_coordinator_impl_browsertest.cc#newcode26
content/browser/memory/memory_coordinator_impl_browsertest.cc:26:
[&state](memory_coordinator::mojom::ChildMemoryCoordinator* child) {

On 2016/07/07 02:53:44, chrisha (slow) wrote:
> Capturing state by ref here, and by value below? Be consistent?

(seems forgot to remove &) Changed to by value.

https://codereview.chromium.org/2094583002/diff/120001/content/browser/memory/memory_coordinator_impl_browsertest.cc#newcode51
content/browser/memory/memory_coordinator_impl_browsertest.cc:51:
child->GetCurrentState(

On 2016/07/07 02:53:44, chrisha (slow) wrote:
> GetCurrentState is only used for testing, no? Maybe better off named
> GetCurrentStateForTesting?

Renamed.

>
> Also, why the need to pass in a callback? Can't it be a simple getter
function?

This is a mojo method and a mojo method takes a callback when it returns
something. Return value is passed to a callback.

https://codereview.chromium.org/2094583002/

[chr...@chromium.org]

Ah, forgive my lack of familiarity with Mojo IPC. lgtm!

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

dcheng@:
Could you do IPC security review?
(comoponents/memory_coordinator/public/interfaces/memory_coordinator.mojom. This
is an empty interface but I'll add methods in follow-up CLs)

jam@:
Could you review for content/ changes and services/shell dependency?

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

LGTM on my side.



https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.cc
File components/memory_coordinator/browser/memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.cc#newcode17
components/memory_coordinator/browser/memory_coordinator_impl.cc:17:
children_.AddPtr(std::move(child));

Don't we need to remove the pointer when the renderer is gone?

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h
File components/memory_coordinator/browser/memory_coordinator_impl.h
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h#newcode27
components/memory_coordinator/browser/memory_coordinator_impl.h:27: void
IterateChildren(FunctionType function) {

Is this only for testing? Or is it going to be used in other code in the
future?

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.cc
File components/memory_coordinator/browser/memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.cc#newcode17
components/memory_coordinator/browser/memory_coordinator_impl.cc:17:
children_.AddPtr(std::move(child));

On 2016/07/08 02:40:25, haraken wrote:
>
> Don't we need to remove the pointer when the renderer is gone?

No. InterfacePtrSet removes it when an error occurred on the connection.

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h
File components/memory_coordinator/browser/memory_coordinator_impl.h
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h#newcode27
components/memory_coordinator/browser/memory_coordinator_impl.h:27: void
IterateChildren(FunctionType function) {

On 2016/07/08 02:40:26, haraken wrote:
>
> Is this only for testing? Or is it going to be used in other code in
the future?
>

Yes. I don't have plans to use it outside tests.

https://codereview.chromium.org/2094583002/

[har...@chromium.org]

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h
File components/memory_coordinator/browser/memory_coordinator_impl.h
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h#newcode27
components/memory_coordinator/browser/memory_coordinator_impl.h:27: void
IterateChildren(FunctionType function) {

On 2016/07/08 02:44:49, bashi1 wrote:
> On 2016/07/08 02:40:26, haraken wrote:
> >
> > Is this only for testing? Or is it going to be used in other code in
the
> future?
> >
>
> Yes. I don't have plans to use it outside tests.

Then IterateChildrenForTesting?

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h
File components/memory_coordinator/browser/memory_coordinator_impl.h
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/browser/memory_coordinator_impl.h#newcode27
components/memory_coordinator/browser/memory_coordinator_impl.h:27: void
IterateChildren(FunctionType function) {

On 2016/07/08 02:48:54, haraken wrote:
> On 2016/07/08 02:44:49, bashi1 wrote:
> > On 2016/07/08 02:40:26, haraken wrote:
> > >
> > > Is this only for testing? Or is it going to be used in other code
in the
> > future?
> > >
> >
> > Yes. I don't have plans to use it outside tests.
>
> Then IterateChildrenForTesting?

Will do.

https://codereview.chromium.org/2094583002/

[dch...@chromium.org]

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/child/child_memory_coordinator_impl.cc
File
components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode18
components/memory_coordinator/child/child_memory_coordinator_impl.cc:18:
binding_.Bind(std::move(request));
I think it is somewhat unusual that we have a persistent object (scoped
to lifetime of RenderThreadImpl) that only accepts one binding request.
Logically, this is OK, since the browser process should only ever
connect once, but it feels a bit weird. I am going to ask chromium-mojo
about best practices for this.

https://codereview.chromium.org/2094583002/diff/140001/content/renderer/render_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2094583002/diff/140001/content/renderer/render_thread_impl.cc#newcode823
content/renderer/render_thread_impl.cc:823:
base::Unretained(memory_coordinator_.get())));
Does it make sense to just have memory_coordinator_ be a base::Owned()
of the callback?

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/child/child_memory_coordinator_impl.cc
File
components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2094583002/diff/140001/components/memory_coordinator/child/child_memory_coordinator_impl.cc#newcode18
components/memory_coordinator/child/child_memory_coordinator_impl.cc:18:
binding_.Bind(std::move(request));

On 2016/07/08 07:10:35, dcheng wrote:
> I think it is somewhat unusual that we have a persistent object
(scoped to
> lifetime of RenderThreadImpl) that only accepts one binding request.
Logically,
> this is OK, since the browser process should only ever connect once,
but it
> feels a bit weird. I am going to ask chromium-mojo about best
practices for
> this.

Yeah.. Actually, in PS#5 I didn't use this pattern (I guess PS#5 is a
usual way). The reason I changed the pattern is that the browser needs
to have RenderProcessHost::GetID() -> ChildMemoryCoordinatorPtr mappings
so that the central MemoryCoordinator can use the logic in TabManager
(which uses RenderProcessHost and old IPC) to dispatch memory pressure
notification to renderers. I intentionally dropped the mappings from
this CL to make this CL simple, but the next CL would add the mappings.
I'm happy to change this if there is a workaround.

https://codereview.chromium.org/2094583002/diff/140001/content/renderer/render_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2094583002/diff/140001/content/renderer/render_thread_impl.cc#newcode823
content/renderer/render_thread_impl.cc:823:
base::Unretained(memory_coordinator_.get())));

On 2016/07/08 07:10:35, dcheng wrote:
> Does it make sense to just have memory_coordinator_ be a base::Owned()
of the
> callback?

I didn't know base::Owned(). Thanks. Probably we can use it (may depend
on how we connect the browser and renderers).

https://codereview.chromium.org/2094583002/

[j...@chromium.org]

lgtm



https://codereview.chromium.org/2094583002/

[j...@chromium.org]

btw do you want this to work for other process types as well, i.e. not just
renderers? can be done in a followup, but I was wondering what your plan is.

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

On 2016/07/09 02:36:13, jam wrote:
> btw do you want this to work for other process types as well, i.e. not just
> renderers? can be done in a followup, but I was wondering what your plan is.

Currently I don't have a concrete plan to work for other process types but other
processes types are our future targets (probably V1 or later).

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

PS#9 changed the way to bind mojo interfaces and its implements. It stops using
a mojo interface for MemoryCoordinator. Instead, PS#9 adds
MemoryCoordinatorHandle mojo interface which will be created per renderer by the
browser. MemoryCoordinator manages ID -> handle mappings. This way we don't need
to register ChildMemoryCoordinator as an exposed interface in renderers.

dcheng@, does this make sense to you?


https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/memory_coordinator.mojom#newcode11
components/memory_coordinator/public/interfaces/memory_coordinator.mojom:11:
interface MemoryCoordinatorHandle {
Maybe "handle" isn't a good naming for this interface. I couldn't come
up with better name.

https://codereview.chromium.org/2094583002/

[dch...@chromium.org]

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h
File components/memory_coordinator/browser/memory_coordinator.h (right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h#newcode23
components/memory_coordinator/browser/memory_coordinator.h:23: void
CreateHandle(int id, mojom::MemoryCoordinatorHandleRequest request);
Nit: please call this rph_id or something so it's clearer what kind of
ID it is.

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h#newcode27
components/memory_coordinator/browser/memory_coordinator.h:27: void
IterateChildrenForTesting(IterateCallback callback);
Nit: pass callbacks by const ref. But rather than having this, I think
we should just friend a test helper that can access the state: it will
make the tests very straightforward, since all the test code will be
inline.

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h#newcode32
components/memory_coordinator/browser/memory_coordinator.h:32:
std::map<int, std::unique_ptr<MemoryCoordinatorHandleImpl>> children_;
And please add a comment here =)

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom#newcode24
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24:
GetCurrentStateForTesting() => (MemoryState state);
I think we shouldn't expose testing interfaces in the mojom if there are
other ways. This would be akin to adding an IPC just for testing
purposes: it's another IPC to audit for security and isn't something we
want to encourage.

It looks like all the tests that use this can just use the impl directly
instead: I think doing that would make the tests simpler as well (since
we don't need to wait for all the callbacks to complete).

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

Thank you for review!

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h
File components/memory_coordinator/browser/memory_coordinator.h (right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h#newcode23
components/memory_coordinator/browser/memory_coordinator.h:23: void
CreateHandle(int id, mojom::MemoryCoordinatorHandleRequest request);

On 2016/07/12 05:20:28, dcheng wrote:
> Nit: please call this rph_id or something so it's clearer what kind of
ID it is.

Done.

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h#newcode27
components/memory_coordinator/browser/memory_coordinator.h:27: void
IterateChildrenForTesting(IterateCallback callback);

On 2016/07/12 05:20:28, dcheng wrote:
> Nit: pass callbacks by const ref. But rather than having this, I think
we should
> just friend a test helper that can access the state: it will make the
tests very
> straightforward, since all the test code will be inline.

Hmm, can we make the test helper a friend of this class? The test lives
in content/ but this lives in components/memory_coordinator. Dependency
is content/ -> components/memory_coordinator.

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/browser/memory_coordinator.h#newcode32
components/memory_coordinator/browser/memory_coordinator.h:32:
std::map<int, std::unique_ptr<MemoryCoordinatorHandleImpl>> children_;

On 2016/07/12 05:20:28, dcheng wrote:
> And please add a comment here =)

Done.

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom#newcode24
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24:
GetCurrentStateForTesting() => (MemoryState state);

On 2016/07/12 05:20:28, dcheng wrote:
> I think we shouldn't expose testing interfaces in the mojom if there
are other
> ways. This would be akin to adding an IPC just for testing purposes:
it's
> another IPC to audit for security and isn't something we want to
encourage.
>
> It looks like all the tests that use this can just use the impl
directly
> instead: I think doing that would make the tests simpler as well
(since we don't
> need to wait for all the callbacks to complete).

Maybe I'm wrong but I think we need an IPC to get information from impl
class because an instance of the impl class is in renderer and the test
added in this CL is a browsertest. Is there a way to access impl
instances which live in a renderer from a browsertest?

https://codereview.chromium.org/2094583002/

[dch...@chromium.org]

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom#newcode24
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24:
GetCurrentStateForTesting() => (MemoryState state);

On 2016/07/12 05:51:46, bashi1 wrote:
> On 2016/07/12 05:20:28, dcheng wrote:
> > I think we shouldn't expose testing interfaces in the mojom if there
are other
> > ways. This would be akin to adding an IPC just for testing purposes:
it's
> > another IPC to audit for security and isn't something we want to
encourage.
> >
> > It looks like all the tests that use this can just use the impl
directly
> > instead: I think doing that would make the tests simpler as well
(since we
> don't
> > need to wait for all the callbacks to complete).
>
> Maybe I'm wrong but I think we need an IPC to get information from
impl class
> because an instance of the impl class is in renderer and the test
added in this
> CL is a browsertest. Is there a way to access impl instances which
live in a
> renderer from a browsertest?

Ah, I missed that it's a browser test. I think we still don't want to
expose test only IPCs if we can avoid it... as far as I know of, we only
have 3 in legacy IPC and it'd be good not to add another. Hmm...

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom#newcode24
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24:
GetCurrentStateForTesting() => (MemoryState state);

I see. Let me think about a workaround. Probably we just check if there
is a mapping in the browser to test renderers create their
ChildMemoryCoordinator.

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
File
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom
(right):

https://codereview.chromium.org/2094583002/diff/160001/components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom#newcode24
components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom:24:
GetCurrentStateForTesting() => (MemoryState state);

Done. dcheng@, what do you think about PS#11?

https://codereview.chromium.org/2094583002/

[dch...@chromium.org]

LGTM.

I'm not wholly satisfied in terms of clarify of lifetimes here, but that's
probably a bigger question that needs to be tackled outside the scope of this
CL.


https://codereview.chromium.org/2094583002/diff/200001/content/browser/renderer_host/render_process_host_impl.cc
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2094583002/diff/200001/content/browser/renderer_host/render_process_host_impl.cc#newcode454
content/browser/renderer_host/render_process_host_impl.cc:454: int id,
Nit: render_process_id

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

> I'm not wholly satisfied in terms of clarify of lifetimes here, but that's
> probably a bigger question that needs to be tackled outside the scope of this
> CL.

Added a TODO in RenderThreadImpl.

chrisha@, jam@, haraken@:
The latest PS has non-trivial changes since you reviewed. I'll land this CL
tomorrow (or maybe day after tomorrow). Please respond if you have concerns.
Thanks!

https://codereview.chromium.org/2094583002/diff/200001/content/browser/renderer_host/render_process_host_impl.cc
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2094583002/diff/200001/content/browser/renderer_host/render_process_host_impl.cc#newcode454
content/browser/renderer_host/render_process_host_impl.cc:454: int id,

On 2016/07/12 07:44:46, dcheng wrote:
> Nit: render_process_id

Done.

https://codereview.chromium.org/2094583002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2094583002/240001


https://codereview.chromium.org/2094583002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #13 (id:240001)

https://codereview.chromium.org/2094583002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ bit was unchecked.

https://codereview.chromium.org/2094583002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 13 (id:??) landed as
https://crrev.com/066dcbbad57a70ba5380a4a8cdd0572d89bdc279
Cr-Commit-Position: refs/heads/master@{#405402}

https://codereview.chromium.org/2094583002/

[ba...@chromium.org]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2143353004/ by ba...@chromium.org.

The reason for reverting is: Broke mac build.

https://codereview.chromium.org/2094583002/