Expose DiscardTab() to MemoryCoordiantor

Expose DiscardTab() to MemoryCoordiantor
    
Expose TabManager::DiscardTab() to MemoryCoordinator via
MemoryCoordinatorDelegate so that MemoryCoordinator can trigger tab
discarding. This is a preparation for MC/tab discarding integration[1].

[1]
https://docs.google.com/document/d/1QZb1qKR0Jr0pnfy-kks7Cj5rk4wm-cPHRJskQeLbZlw/edit#heading=h.1h4dixj922dt

BUG=689363
Review-Url: https://codereview.chromium.org/2681643003
Cr-Commit-Position: refs/heads/master@{#452731}
Committed: https://chromium.googlesource.com/chromium/src/+/8c4c9243dfc3d8ef05562ae4e3f858956ec4ae14

[bashi, 2017-02-07 07:04:52]

Description was changed from

==========
WIP: Tab discarding with memory coordinator

Expose TabManager::DiscardTab() to MemoryCoordinator via
MemoryCoordinatorDelegate. The core logic of tab discarding remains
TabManager. MemoryCoordinator calls DiscardTab() if
expected_renderer_count < N (where N = 3 in this CL; just a guess).
This check occurs every time we update available free memory, which
means:
- every 5 seconds when no memory state transition
- 30 seconds after from the last memory state transition

BUG=689363
==========

to

==========
WIP: Tab discarding with memory coordinator

Expose TabManager::DiscardTab() to MemoryCoordinator via
MemoryCoordinatorDelegate. The core logic of tab discarding remains
TabManager. MemoryCoordinator calls DiscardTab() if
expected_renderer_count <= N (where N = 3 in this CL; just a guess).
This check occurs every time we update available free memory, which
means:
- every 5 seconds when no memory state transition
- 30 seconds after from the last memory state transition

BUG=689363
==========

[bashi, 2017-02-07 07:09:18]

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

[bashi, 2017-02-07 07:09:18]

Hi Chris,

Does this approach make sense to you? Or should we try to factor out tab
discarding logic from TabManager to somewhere and use it from
TabManager/MemoryCoordinator?

This is just an idea so please let me know if you have other ideas. Thanks!

[haraken, 2017-02-07 07:24:47]

On 2017/02/07 07:09:18, bashi wrote:
> Hi Chris,
> 
> Does this approach make sense to you? Or should we try to factor out tab
> discarding logic from TabManager to somewhere and use it from
> TabManager/MemoryCoordinator?
> 
> This is just an idea so please let me know if you have other ideas. Thanks!

This makes sense to me.

[bashi, 2017-02-17 05:50:13]

Description was changed from

==========
WIP: Tab discarding with memory coordinator

Expose TabManager::DiscardTab() to MemoryCoordinator via
MemoryCoordinatorDelegate. The core logic of tab discarding remains
TabManager. MemoryCoordinator calls DiscardTab() if
expected_renderer_count <= N (where N = 3 in this CL; just a guess).
This check occurs every time we update available free memory, which
means:
- every 5 seconds when no memory state transition
- 30 seconds after from the last memory state transition

BUG=689363
==========

to

==========
Expose DiscardTab() to MemoryCoordiantor
    
Expose TabManager::DiscardTab() to MemoryCoordinator via
MemoryCoordinatorDelegate so that MemoryCoordinator can trigger tab
discarding. This is a preparation for MC/tab discarding integration[1].

[1]
https://docs.google.com/document/d/1QZb1qKR0Jr0pnfy-kks7Cj5rk4wm-cPHRJskQeLbZ...

BUG=689363
==========

[bashi, 2017-02-17 05:52:28]

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

[commit-bot: I haz the power, 2017-02-17 05:53:03]

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 06:01:57]

Revised. I think this is ready for review.

chrisha@: what do you think about this? fmeawad@ has started servicification of
tab discarding. Until it becomes available I want to expose DiscardTab() so that
we can work in parallel.

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

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

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

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...)

[bashi, 2017-02-17 06:26:32]

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

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

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

[haraken, 2017-02-17 07:20:20]

LGTM on my side

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

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

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

Dry run: This issue passed the CQ dry run.

[bashi, 2017-02-23 02:16:26]

chrisha@: Friendly remainder :)

[chrisha, 2017-02-23 22:30:04]

lgtm

[bashi, 2017-02-23 23:04:03]

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

[bashi, 2017-02-23 23:04:04]

kinuko@: could you review content/ ?

[kinuko, 2017-02-24 01:29:24]

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

https://codereview.chromium.org/2681643003/diff/40001/content/public/browser/...
content/public/browser/memory_coordinator_delegate.h:21: // discard.
Is this called when memory is tight? Is the impl supposed to discard one tab at
most?  Is it allowed not to discard tabs if it can't find any candidate?

I see this actually calls TabManager's corresponding method in chrome case, but
might be better to have a bit more descriptive comment for implementors.

[bashi, 2017-02-24 01:40:26]

Thanks for review!

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

https://codereview.chromium.org/2681643003/diff/40001/content/public/browser/...
content/public/browser/memory_coordinator_delegate.h:21: // discard.
On 2017/02/24 01:29:24, kinuko wrote:
> Is this called when memory is tight? Is the impl supposed to discard one tab
at
> most?  Is it allowed not to discard tabs if it can't find any candidate?
> 
> I see this actually calls TabManager's corresponding method in chrome case,
but
> might be better to have a bit more descriptive comment for implementors.

Updated the comments.

[kinuko, 2017-02-24 01:45:35]

lgtm

[bashi, 2017-02-24 01:52:52]

The CQ bit was checked by bashi@chromium.org

[bashi, 2017-02-24 01:52:52]

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

[commit-bot: I haz the power, 2017-02-24 01:53:34]

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-24 03:12:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487901172488760,
"parent_rev": "026ba0568676344ed6ce3196a5a321abcb9bf3f1", "commit_rev":
"8c4c9243dfc3d8ef05562ae4e3f858956ec4ae14"}

[commit-bot: I haz the power, 2017-02-24 03:12:36]

Description was changed from

==========
Expose DiscardTab() to MemoryCoordiantor
    
Expose TabManager::DiscardTab() to MemoryCoordinator via
MemoryCoordinatorDelegate so that MemoryCoordinator can trigger tab
discarding. This is a preparation for MC/tab discarding integration[1].

[1]
https://docs.google.com/document/d/1QZb1qKR0Jr0pnfy-kks7Cj5rk4wm-cPHRJskQeLbZ...

BUG=689363
==========

to

==========
Expose DiscardTab() to MemoryCoordiantor
    
Expose TabManager::DiscardTab() to MemoryCoordinator via
MemoryCoordinatorDelegate so that MemoryCoordinator can trigger tab
discarding. This is a preparation for MC/tab discarding integration[1].

[1]
https://docs.google.com/document/d/1QZb1qKR0Jr0pnfy-kks7Cj5rk4wm-cPHRJskQeLbZ...

BUG=689363

Review-Url: https://codereview.chromium.org/2681643003
Cr-Commit-Position: refs/heads/master@{#452731}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c4c9243dfc3d8ef05562ae4e3f8...
==========

[commit-bot: I haz the power, 2017-02-24 03:12:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8c4c9243dfc3d8ef05562ae4e3f8...