Refactor MemoryCoordinatorProxy

Refactor MemoryCoordinatorProxy

Before this CL we used two callbacks to inject content -> base
dependency. Now we plan to add another method which will provide
remaining memory budget (WIP CL can be found [1]). Rather than adding
another callback we should simply use an interface to inject dependency.

[1] https://codereview.chromium.org/2479673002/

BUG=693430
Review-Url: https://codereview.chromium.org/2703743002
Cr-Commit-Position: refs/heads/master@{#451573}
Committed: https://chromium.googlesource.com/chromium/src/+/4e0e67fdb9bd900fb4f7d6e7166f60a53800e2ed

[bashi, 2017-02-17 08:33:31]

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

[commit-bot: I haz the power, 2017-02-17 08:33:54]

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

[bashi, 2017-02-17 08:35:12]

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

[bashi, 2017-02-17 08:35:13]

hajimehoshi@: PTAL before asking owners review

[hajimehoshi, 2017-02-17 09:28:30]

lgtm

https://codereview.chromium.org/2703743002/diff/1/content/public/test/memory_...
File content/public/test/memory_coordinator_test_utils.cc (right):

https://codereview.chromium.org/2703743002/diff/1/content/public/test/memory_...
content/public/test/memory_coordinator_test_utils.cc:14: // Make sure that
MemoryCoordinatorImpl is initialized.
This is needed to call base::MemoryCoordinatorProxy::SetMemoryCoordinator in the
constructor and enable base::MemoryCoordinatorProxy, right?

[commit-bot: I haz the power, 2017-02-17 10:46:05]

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

[commit-bot: I haz the power, 2017-02-17 10:46:06]

Dry run: This issue passed the CQ dry run.

[bashi, 2017-02-19 22:57:17]

bashi@chromium.org changed reviewers:
+ chrisha@chromium.org, dcheng@chromium.org, kinuko@chromium.org

[bashi, 2017-02-19 22:57:18]

+dcheng@ for base/ review
+kinuko@ for content/ review

+chrisha@ FYI

https://codereview.chromium.org/2703743002/diff/1/content/public/test/memory_...
File content/public/test/memory_coordinator_test_utils.cc (right):

https://codereview.chromium.org/2703743002/diff/1/content/public/test/memory_...
content/public/test/memory_coordinator_test_utils.cc:14: // Make sure that
MemoryCoordinatorImpl is initialized.
On 2017/02/17 09:28:30, hajimehoshi wrote:
> This is needed to call base::MemoryCoordinatorProxy::SetMemoryCoordinator in
the
> constructor and enable base::MemoryCoordinatorProxy, right?

Yes.

[kinuko, 2017-02-20 02:56:42]

On 2017/02/19 22:57:18, bashi wrote:
> +dcheng@ for base/ review
> +kinuko@ for content/ review

lgtm (as far as base change lgt the owner)

[dcheng, 2017-02-20 03:07:21]

https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
File base/memory/memory_coordinator_proxy.cc (right):

https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_proxy.cc:13: subtle::AtomicWord
g_memory_coordinator = 0;
Is it necessary to use atomics? The base::Callback version of this interface
didn't use them.

[bashi, 2017-02-20 03:17:00]

https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
File base/memory/memory_coordinator_proxy.cc (right):

https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
base/memory/memory_coordinator_proxy.cc:13: subtle::AtomicWord
g_memory_coordinator = 0;
On 2017/02/20 03:07:21, dcheng wrote:
> Is it necessary to use atomics? The base::Callback version of this interface
> didn't use them.

Probably it isn't necessary to use atomics. I just felt that it's a bit unsafe
to set a static variable without guards. Should we remove atomics?

[kinuko, 2017-02-20 03:22:51]

On 2017/02/20 03:17:00, bashi wrote:
>
https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
> File base/memory/memory_coordinator_proxy.cc (right):
> 
>
https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
> base/memory/memory_coordinator_proxy.cc:13: subtle::AtomicWord
> g_memory_coordinator = 0;
> On 2017/02/20 03:07:21, dcheng wrote:
> > Is it necessary to use atomics? The base::Callback version of this interface
> > didn't use them.
> 
> Probably it isn't necessary to use atomics. I just felt that it's a bit unsafe
> to set a static variable without guards. Should we remove atomics?

You could probably document it clearer when and how SetMemoryCoordinator should
be called in .h (e.g. before any other threads are created) and remove the
guards?

[bashi, 2017-02-20 04:26:51]

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

[commit-bot: I haz the power, 2017-02-20 04:27:18]

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

[bashi, 2017-02-20 04:28:01]

On 2017/02/20 03:22:51, kinuko wrote:
> On 2017/02/20 03:17:00, bashi wrote:
> >
>
https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
> > File base/memory/memory_coordinator_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/2703743002/diff/1/base/memory/memory_coordina...
> > base/memory/memory_coordinator_proxy.cc:13: subtle::AtomicWord
> > g_memory_coordinator = 0;
> > On 2017/02/20 03:07:21, dcheng wrote:
> > > Is it necessary to use atomics? The base::Callback version of this
interface
> > > didn't use them.
> > 
> > Probably it isn't necessary to use atomics. I just felt that it's a bit
unsafe
> > to set a static variable without guards. Should we remove atomics?
> 
> You could probably document it clearer when and how SetMemoryCoordinator
should
> be called in .h (e.g. before any other threads are created) and remove the
> guards?

Done. Thanks for the suggestion!

[dcheng, 2017-02-20 04:31:44]

base lgtm

[commit-bot: I haz the power, 2017-02-20 06:10:20]

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

[commit-bot: I haz the power, 2017-02-20 06:10:22]

Dry run: This issue passed the CQ dry run.

[bashi, 2017-02-20 06:12:51]

The CQ bit was checked by bashi@chromium.org

[bashi, 2017-02-20 06:12:51]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
hajimehoshi@chromium.org
Link to the patchset: https://codereview.chromium.org/2703743002/#ps20001
(title: "Remove atomics")

[commit-bot: I haz the power, 2017-02-20 06:12:58]

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, 2017-02-20 06:16:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487571171081360,
"parent_rev": "c88786898b6f37bf802e1db5aee843f3b27c1627", "commit_rev":
"4e0e67fdb9bd900fb4f7d6e7166f60a53800e2ed"}

[commit-bot: I haz the power, 2017-02-20 06:17:17]

Description was changed from

==========
Refactor MemoryCoordinatorProxy

Before this CL we used two callbacks to inject content -> base
dependency. Now we plan to add another method which will provide
remaining memory budget (WIP CL can be found [1]). Rather than adding
another callback we should simply use an interface to inject dependency.

[1] https://codereview.chromium.org/2479673002/

BUG=693430
==========

to

==========
Refactor MemoryCoordinatorProxy

Before this CL we used two callbacks to inject content -> base
dependency. Now we plan to add another method which will provide
remaining memory budget (WIP CL can be found [1]). Rather than adding
another callback we should simply use an interface to inject dependency.

[1] https://codereview.chromium.org/2479673002/

BUG=693430

Review-Url: https://codereview.chromium.org/2703743002
Cr-Commit-Position: refs/heads/master@{#451573}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e0e67fdb9bd900fb4f7d6e7166f...
==========

[commit-bot: I haz the power, 2017-02-20 06:17:19]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4e0e67fdb9bd900fb4f7d6e7166f...