Add a static getter for ChildMemoryCoordinator

Add a static getter for ChildMemoryCoordinator

Currently ChildMemoryCoordinator is owned by RenderThreadImpl but
some other components (like cc) would want to become clients of
ChildMemoryCoordinator. Add a static getter to provide access
to ChildMemoryCoordinator.

BUG=617492
Committed: https://crrev.com/7056550d52a4e61bd120c24950d421fb0032dc55
Cr-Commit-Position: refs/heads/master@{#415212}

[bashi, 2016-08-29 09:45:39]

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

[commit-bot: I haz the power, 2016-08-29 09:45:55]

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

[bashi, 2016-08-29 09:52:06]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, tasak@google.com

[bashi, 2016-08-29 09:52:07]

PTAL

Per request (https://codereview.chromium.org/2286583002/)

https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
File components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
components/memory_coordinator/child/child_memory_coordinator_impl.cc:14:
base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
Not sure we really need a lock. Can be removed when all callsites access the
getter only RenderTreadImpl is alive.

[tasak, 2016-08-29 09:56:09]

Thank you, bashi-san.

lgtm

[haraken, 2016-08-29 10:00:37]

LGTM

[bashi, 2016-08-29 10:02:52]

The CQ bit was unchecked by bashi@chromium.org

[bashi, 2016-08-29 10:02:55]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-08-29 10:03:07]

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-08-29 10:44:27]

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

[commit-bot: I haz the power, 2016-08-29 10:44:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bashi, 2016-08-29 22:49:15]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-08-30 02:13:01]

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-08-30 03:42:51]

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

[commit-bot: I haz the power, 2016-08-30 03:42:52]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bashi, 2016-08-30 04:38:06]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2016-08-30 04:40:15]

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-08-30 06:18:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-30 06:19:53]

Description was changed from

==========
Add a static getter for ChildMemoryCoordinator

Currently ChildMemoryCoordinator is owned by RenderThreadImpl but
some other components (like cc) would want to become clients of
ChildMemoryCoordinator. Add a static getter to provide access
to ChildMemoryCoordinator.

BUG=617492
==========

to

==========
Add a static getter for ChildMemoryCoordinator

Currently ChildMemoryCoordinator is owned by RenderThreadImpl but
some other components (like cc) would want to become clients of
ChildMemoryCoordinator. Add a static getter to provide access
to ChildMemoryCoordinator.

BUG=617492

Committed: https://crrev.com/7056550d52a4e61bd120c24950d421fb0032dc55
Cr-Commit-Position: refs/heads/master@{#415212}
==========

[commit-bot: I haz the power, 2016-08-30 06:19:54]

Patchset 1 (id:??) landed as
https://crrev.com/7056550d52a4e61bd120c24950d421fb0032dc55
Cr-Commit-Position: refs/heads/master@{#415212}

[haraken, 2016-09-01 18:49:10]

https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
File components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
components/memory_coordinator/child/child_memory_coordinator_impl.cc:14:
base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
On 2016/08/29 09:52:06, bashi1 wrote:
> Not sure we really need a lock. Can be removed when all callsites access the
> getter only RenderTreadImpl is alive.

I begin to think that ChildMemoryCoordinator must be accessed only on the main
thread. Calling RegisterClient/UnregisterClient on a non-main thread would not
be a safe thing. Even if we make the registration/unregistration thread-safe,
it's still not safe to call OnMemoryStateChange on a non-main thread.

So I think we should add DCHECK(isMainThread()) instead of adding a lock.

[bashi, 2016-09-01 22:53:39]

https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
File components/memory_coordinator/child/child_memory_coordinator_impl.cc
(right):

https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
components/memory_coordinator/child/child_memory_coordinator_impl.cc:14:
base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
On 2016/09/01 18:49:10, haraken wrote:
> On 2016/08/29 09:52:06, bashi1 wrote:
> > Not sure we really need a lock. Can be removed when all callsites access the
> > getter only RenderTreadImpl is alive.
> 
> I begin to think that ChildMemoryCoordinator must be accessed only on the main
> thread. Calling RegisterClient/UnregisterClient on a non-main thread would not
> be a safe thing. Even if we make the registration/unregistration thread-safe,
> it's still not safe to call OnMemoryStateChange on a non-main thread.
> 
> So I think we should add DCHECK(isMainThread()) instead of adding a lock.
> 

Making ChildMemoryCoordinator only accessible via the main thread seems fine to
me, but registerClient/unregisterClient is thread-safe now as we use
base::ObserverListThreadSafe<MemoryCoordinatorClient>. OnStateChange() will be
called on the thread registerClient() is called.

At this point I don't think we need DCHECK(isMainThread()).

If we want CMC be accessible only on the main thread, we need to provide a way
to specify which thread OnStateChange() should be called.

[haraken, 2016-09-01 23:20:29]

On 2016/09/01 22:53:39, bashi1 wrote:
>
https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
> File components/memory_coordinator/child/child_memory_coordinator_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2291473002/diff/1/components/memory_coordinat...
> components/memory_coordinator/child/child_memory_coordinator_impl.cc:14:
> base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
> On 2016/09/01 18:49:10, haraken wrote:
> > On 2016/08/29 09:52:06, bashi1 wrote:
> > > Not sure we really need a lock. Can be removed when all callsites access
the
> > > getter only RenderTreadImpl is alive.
> > 
> > I begin to think that ChildMemoryCoordinator must be accessed only on the
main
> > thread. Calling RegisterClient/UnregisterClient on a non-main thread would
not
> > be a safe thing. Even if we make the registration/unregistration
thread-safe,
> > it's still not safe to call OnMemoryStateChange on a non-main thread.
> > 
> > So I think we should add DCHECK(isMainThread()) instead of adding a lock.
> > 
> 
> Making ChildMemoryCoordinator only accessible via the main thread seems fine
to
> me, but registerClient/unregisterClient is thread-safe now as we use
> base::ObserverListThreadSafe<MemoryCoordinatorClient>. OnStateChange() will be
> called on the thread registerClient() is called.
> 
> At this point I don't think we need DCHECK(isMainThread()).
> 
> If we want CMC be accessible only on the main thread, we need to provide a way
> to specify which thread OnStateChange() should be called.

Sorry, I was misunderstanding. Let's keep the current code as is -- it's clearly
better to support multi-threads in CMC :)