Add MemoryCoordinatorDelegate

Add MemoryCoordinatorDelegate

Memory coordinator requires some information only avaiable in
chrome/browser. One option to work around content -> chrome dependency
is to put an abstract class in content/public/broser and an impl class in
chrome/browser. However, We want to keep browser-agnostic things in
content as much as possible. This CL introduces a delegate class to
keep things in content but also provide ways to access information in
chrome/browser.

BUG=617492
Committed: https://crrev.com/93362955b0109abee326322a952989fd5ee0d0f2
Cr-Commit-Position: refs/heads/master@{#425202}

[bashi, 2016-10-07 04:03:40]

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

[bashi, 2016-10-07 04:03:41]

(This comes from 
https://codereview.chromium.org/2374343002/diff/20001/content/browser/memory/...)

Hi Chris, what do you think about this? If this looks good to you, I'll try to
add tests.

Making TabManager be the delegate of MemoryCoordinator is a tentative solution.
If we go this direction, we will implement a dedicated delegate class in the
end.

https://codereview.chromium.org/2399293002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2399293002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:424: for (auto& tab : tab_stats) {
This isn't efficient but I couldn't figure out how to get WebContents (or ids of
WebContent) from render_process_id.

(My guess is that there are 1:N relationship between render_process_host and
web_content. Haven't looked at the implementation though)

https://codereview.chromium.org/2399293002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:427: if
(!CanDiscardTab(tab.tab_contents_id))
Or we may want to check only IsMediaTab()

[haraken, 2016-10-07 06:38:19]

https://codereview.chromium.org/2399293002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2399293002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:424: for (auto& tab : tab_stats) {
On 2016/10/07 04:03:41, bashi1 wrote:
> This isn't efficient but I couldn't figure out how to get WebContents (or ids
of
> WebContent) from render_process_id.
> 
> (My guess is that there are 1:N relationship between render_process_host and
> web_content. Haven't looked at the implementation though)

Yeah, the underlying issue is that we have a way to suspend a renderer but don't
have a way to suspend a tab.

Once we move almost everything to per-frame schedulers, maybe we can associate
the per-frame schedulers with a tab, which will enable us to suspend tasks of a
specific tab. However, it will be an amount of work.

https://codereview.chromium.org/2399293002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:427: if
(!CanDiscardTab(tab.tab_contents_id))
On 2016/10/07 04:03:41, bashi1 wrote:
> Or we may want to check only IsMediaTab()

Yes, but we can fix this later.

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

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator.cc:87: if (iter->second.memory_state
== mojom::MemoryState::SUSPENDED &&

Shouldn't this be:

  if (memory_state == mojom::MemoryState::SUSPENDED && ...)

?

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator.h (right):

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator.h:97:
base::WeakPtr<MemoryCoordinatorDelegate> delegate_;

Why does this need to be a WeakPtr?

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

https://codereview.chromium.org/2399293002/diff/1/content/public/browser/memo...
content/public/browser/memory_coordinator_delegate.h:18: // Returns true when a
given render process can be suspended.

renderer

[bashi, 2016-10-07 07:13:37]

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

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator.cc:87: if (iter->second.memory_state
== mojom::MemoryState::SUSPENDED &&
On 2016/10/07 06:38:19, haraken wrote:
> 
> Shouldn't this be:
> 
>   if (memory_state == mojom::MemoryState::SUSPENDED && ...)
> 
> ?

Yes. Thanks. Done.

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator.h (right):

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator.h:97:
base::WeakPtr<MemoryCoordinatorDelegate> delegate_;
On 2016/10/07 06:38:19, haraken wrote:
> 
> Why does this need to be a WeakPtr?

TabManager (which is effectively the only implementation of the delegate) isn't
a permanent object and we can't control its life cycle from here. Passing a raw
pointer is dangerous.

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

https://codereview.chromium.org/2399293002/diff/1/content/public/browser/memo...
content/public/browser/memory_coordinator_delegate.h:18: // Returns true when a
given render process can be suspended.
On 2016/10/07 06:38:19, haraken wrote:
> 
> renderer

Done.

[haraken, 2016-10-07 07:31:07]

LGTM on my side.

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator.h (right):

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator.h:97:
base::WeakPtr<MemoryCoordinatorDelegate> delegate_;
On 2016/10/07 07:13:37, bashi1 wrote:
> On 2016/10/07 06:38:19, haraken wrote:
> > 
> > Why does this need to be a WeakPtr?
> 
> TabManager (which is effectively the only implementation of the delegate)
isn't
> a permanent object and we can't control its life cycle from here. Passing a
raw
> pointer is dangerous.

I'll defer the decision to Chris -- how the lifetime relationship between MC and
TabManager should be.

[bashi, 2016-10-07 07:34:01]

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator.h (right):

https://codereview.chromium.org/2399293002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator.h:97:
base::WeakPtr<MemoryCoordinatorDelegate> delegate_;
On 2016/10/07 07:31:07, haraken wrote:
> On 2016/10/07 07:13:37, bashi1 wrote:
> > On 2016/10/07 06:38:19, haraken wrote:
> > > 
> > > Why does this need to be a WeakPtr?
> > 
> > TabManager (which is effectively the only implementation of the delegate)
> isn't
> > a permanent object and we can't control its life cycle from here. Passing a
> raw
> > pointer is dangerous.
> 
> I'll defer the decision to Chris -- how the lifetime relationship between MC
and
> TabManager should be.

Just to clarify: This is a tentative workaround. When we move logic from
TabManager to a separate delegate, we can MC owns the delegate.

[chrisha, 2016-10-10 23:25:08]

mostly lgtm, modulo CanSuspendRenderer implementation. I'm fine with the
delegate. There's more logic we can push into this once MC also takes over
discard duties.

https://codereview.chromium.org/2399293002/diff/20001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2399293002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:427: if
(!CanDiscardTab(tab.tab_contents_id))
Discard and suspend have very different semantics. We should be able to suspend
anything that doesn't have a "foreground" presence (ie, casting, playing audio,
etc).

Take a look at tasak's first stab at this here:

https://codereview.chromium.org/2389683002/

[bashi, 2016-10-11 00:54:09]

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

[commit-bot: I haz the power, 2016-10-11 00:54:28]

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

[bashi, 2016-10-11 01:08:28]

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

[commit-bot: I haz the power, 2016-10-11 01:08:50]

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

[bashi, 2016-10-11 01:10:06]

Thanks for review!

https://codereview.chromium.org/2399293002/diff/20001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager.cc (right):

https://codereview.chromium.org/2399293002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:427: if
(!CanDiscardTab(tab.tab_contents_id))
On 2016/10/10 23:25:08, chrisha (slow) wrote:
> Discard and suspend have very different semantics. We should be able to
suspend
> anything that doesn't have a "foreground" presence (ie, casting, playing
audio,
> etc).
> 
> Take a look at tasak's first stab at this here:
> 
> https://codereview.chromium.org/2389683002/

Agreed. Changed to check IsMediaTab().

[bashi, 2016-10-11 01:12:36]

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

[bashi, 2016-10-11 01:12:36]

jam@: could you review content/public/browser ?

[commit-bot: I haz the power, 2016-10-11 01:38:14]

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

[commit-bot: I haz the power, 2016-10-11 01:38:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bashi, 2016-10-11 04:13:28]

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

[commit-bot: I haz the power, 2016-10-11 04:13:38]

Dry run: 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-11 05:40:53]

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

[commit-bot: I haz the power, 2016-10-11 05:40:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jam, 2016-10-11 15:50:00]

https://codereview.chromium.org/2399293002/diff/80001/content/public/browser/...
File content/public/browser/memory_coordinator_delegate.h (right):

https://codereview.chromium.org/2399293002/diff/80001/content/public/browser/...
content/public/browser/memory_coordinator_delegate.h:17: static void Set(const
base::WeakPtr<MemoryCoordinatorDelegate>& delegate);
nit: the general pattern used is for ContentBrowerClient to have a
CreateMemoryCoordinatorDelegate method, instead of a setter here. that way it's
clear to the embedder:
1) when it should create/set it's delegate (i.e. so it's not set too late)
2) to have one place where the embedder can see all the delegates it can create

with that simple change, lgtm

[bashi, 2016-10-13 01:40:56]

I'm going to submit this CL.

Chris, let me know if you have more comments or suggestions. I'll address them
in follow-up CLs as needed.

https://codereview.chromium.org/2399293002/diff/80001/content/public/browser/...
File content/public/browser/memory_coordinator_delegate.h (right):

https://codereview.chromium.org/2399293002/diff/80001/content/public/browser/...
content/public/browser/memory_coordinator_delegate.h:17: static void Set(const
base::WeakPtr<MemoryCoordinatorDelegate>& delegate);
On 2016/10/11 15:50:00, jam wrote:
> nit: the general pattern used is for ContentBrowerClient to have a
> CreateMemoryCoordinatorDelegate method, instead of a setter here. that way
it's
> clear to the embedder:
> 1) when it should create/set it's delegate (i.e. so it's not set too late)
> 2) to have one place where the embedder can see all the delegates it can
create
> 
> with that simple change, lgtm

Done. I was looking for something like ContentBrowserClient. Thanks for the
information!

[bashi, 2016-10-13 01:41:16]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-10-13 01:41:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, chrisha@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2399293002/#ps100001
(title: "Use ContentBrowserClient")

[commit-bot: I haz the power, 2016-10-13 01:41:31]

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-13 02:06:54]

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

[commit-bot: I haz the power, 2016-10-13 02:06:55]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bashi, 2016-10-13 02:19:30]

On 2016/10/13 02:06:55, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

Looks like TabManager is not available on Android. I'll create a separate
delegate class to work around this (we need it sooner or later).

[bashi, 2016-10-13 03:02:22]

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

[commit-bot: I haz the power, 2016-10-13 03:02:46]

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

[bashi, 2016-10-13 03:04:00]

chrisha@, jam@: The latest patch set has non-trivial changes since you reviewed.
Could you take another look?

[commit-bot: I haz the power, 2016-10-13 04:05:17]

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

[commit-bot: I haz the power, 2016-10-13 04:05:18]

Dry run: This issue passed the CQ dry run.

[bashi, 2016-10-13 23:27:30]

As there are some CLs[1,2] which depend on this CL, let me submit this. Please
let me know if you have comments/suggestions. I'll address them in follow-up
CLs.

[1] https://codereview.chromium.org/2374343002/
[2] https://codereview.chromium.org/2412413002/

[bashi, 2016-10-13 23:27:49]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-10-13 23:27:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jam@chromium.org, chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2399293002/#ps120001
(title: "Add a separate delegate class")

[commit-bot: I haz the power, 2016-10-13 23:28:18]

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-13 23:33:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-13 23:34:48]

Description was changed from

==========
Add MemoryCoordinatorDelegate

Memory coordinator requires some information only avaiable in
chrome/browser. One option to work around content -> chrome dependency
is to put an abstract class in content/public/broser and an impl class in
chrome/browser. However, We want to keep browser-agnostic things in
content as much as possible. This CL introduces a delegate class to
keep things in content but also provide ways to access information in
chrome/browser.

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

to

==========
Add MemoryCoordinatorDelegate

Memory coordinator requires some information only avaiable in
chrome/browser. One option to work around content -> chrome dependency
is to put an abstract class in content/public/broser and an impl class in
chrome/browser. However, We want to keep browser-agnostic things in
content as much as possible. This CL introduces a delegate class to
keep things in content but also provide ways to access information in
chrome/browser.

BUG=617492

Committed: https://crrev.com/93362955b0109abee326322a952989fd5ee0d0f2
Cr-Commit-Position: refs/heads/master@{#425202}
==========

[commit-bot: I haz the power, 2016-10-13 23:34:50]

Patchset 7 (id:??) landed as
https://crrev.com/93362955b0109abee326322a952989fd5ee0d0f2
Cr-Commit-Position: refs/heads/master@{#425202}