Modify PurgeAndSuspend to see WebContents

Modify PurgeAndSuspend to see WebContents

- If a background renderer has no media content,
the renderer can be purged and suspended.
- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
 https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit?usp=sharing
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419

[tasak, 2016-10-03 06:22:19]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 06:22:35]

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

[tasak, 2016-10-03 06:27:29]

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

[tasak, 2016-10-03 06:27:30]

Would you review this CL?

[haraken, 2016-10-03 06:32:34]

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

[haraken, 2016-10-03 06:32:35]

Looks good on my side.

chrisha@ should take a look at this.

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

https://codereview.chromium.org/2389683002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:687: if
(web_contents->GetContentsMimeType() == "application/pdf")

I don't think this check is needed. It will be safe to suspend application/pdf
tabs (although I'm not sure how much memory we can save by suspending them).

https://codereview.chromium.org/2389683002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:694: 

I'd add the minimum_protection_time_ check we have in TabManager::CanDiscardTab.

[hajimehoshi, 2016-10-03 06:45:38]

Would you add more description?

[tasak, 2016-10-03 07:01:19]

Description was changed from

==========
Implement CanPurgeAndSuspendBackgroundedTab.

BUG=635419
==========

to

==========
Implement CanPurgeAndSuspendBackgroundedTab.

If a background renderer has some web content that matches the following
condition, the renderer can be purged and suspended:
- no media content,
- not recently used (c.f. minimum protection time),
- not disallowed to discard automatically (reusing tab discarding logic)

BUG=635419
==========

[tasak, 2016-10-03 07:01:29]

Description was changed from

==========
Implement CanPurgeAndSuspendBackgroundedTab.

If a background renderer has some web content that matches the following
condition, the renderer can be purged and suspended:
- no media content,
- not recently used (c.f. minimum protection time),
- not disallowed to discard automatically (reusing tab discarding logic)

BUG=635419
==========

to

==========
Implement CanPurgeAndSuspendBackgroundedTab.

If a background renderer has some web content that matches the following
condition,
the renderer can be purged and suspended:
- no media content,
- not recently used (c.f. minimum protection time),
- not disallowed to discard automatically (reusing tab discarding logic)

BUG=635419
==========

[tasak, 2016-10-03 07:08:28]

On 2016/10/03 06:45:38, hajimehoshi wrote:
> Would you add more description?

Sure. Done.

[tasak, 2016-10-03 07:08:54]

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

https://codereview.chromium.org/2389683002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:687: if
(web_contents->GetContentsMimeType() == "application/pdf")
On 2016/10/03 06:32:35, haraken wrote:
> 
> I don't think this check is needed. It will be safe to suspend application/pdf
> tabs (although I'm not sure how much memory we can save by suspending them).

Done.

https://codereview.chromium.org/2389683002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.cc:694: 
On 2016/10/03 06:32:35, haraken wrote:
> 
> I'd add the minimum_protection_time_ check we have in
TabManager::CanDiscardTab.

Done.

[tasak, 2016-10-03 07:14:02]

Description was changed from

==========
Implement CanPurgeAndSuspendBackgroundedTab.

If a background renderer has some web content that matches the following
condition,
the renderer can be purged and suspended:
- no media content,
- not recently used (c.f. minimum protection time),
- not disallowed to discard automatically (reusing tab discarding logic)

BUG=635419
==========

to

==========
Modify PurgeAndSuspend to see WebContents

If a background renderer has some web content that matches the following
condition,
the renderer can be purged and suspended:
- no media content,
- not recently used (c.f. minimum protection time),
- not disallowed to discard automatically (reusing tab discarding logic)

BUG=635419
==========

[tasak, 2016-10-04 05:33:08]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 05:33:14]

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-04 06:14:49]

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

[commit-bot: I haz the power, 2016-10-04 06:14:50]

Dry run: This issue passed the CQ dry run.

[tasak, 2016-10-04 09:19:19]

tasak@google.com changed reviewers:
+ georgesak@chromium.org

[tasak, 2016-10-04 09:19:38]

Would you review this CL?

[haraken, 2016-10-04 09:33:53]

LGTM on my side.

[hajimehoshi, 2016-10-04 09:36:00]

non-owner lgtm

[Georges Khalil, 2016-10-04 13:28:56]

LGTM % nits.

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

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:687: // Do not discard a recently used tab.
nit: s/discard/suspend.

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:692: return false;
nit: Let's move this logic to a function, instead of duplicating it. Something
like "IsTabTimeProtected".

[chrisha, 2016-10-04 21:52:42]

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

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:684: if (IsMediaTab(web_contents))
This is the only reason I think we shouldn't suspend: the tab is effectively a
foreground tab (visible, or playing audio).

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:687: // Do not discard a recently used tab.
On 2016/10/04 13:28:55, Georges Khalil wrote:
> nit: s/discard/suspend.

I don't think this is necessary. We should feel free to suspend recently used
tabs, as the cost of unsuspending them is effectively nothing (as shown in early
testing).

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:697: if
(!IsTabAutoDiscardable(web_contents))
Discarding logic shouldn't affect suspending, IMO.

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

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:248: // suspended by purge+susend
backgrounded tab mechanism.
suspend*

[tasak, 2016-10-05 05:28:46]

Thank you for review.

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

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:684: if (IsMediaTab(web_contents))
On 2016/10/04 21:52:41, chrisha (slow) wrote:
> This is the only reason I think we shouldn't suspend: the tab is effectively a
> foreground tab (visible, or playing audio).

I see.
Done.

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:687: // Do not discard a recently used tab.
On 2016/10/04 21:52:41, chrisha (slow) wrote:
> On 2016/10/04 13:28:55, Georges Khalil wrote:
> > nit: s/discard/suspend.
> 
> I don't think this is necessary. We should feel free to suspend recently used
> tabs, as the cost of unsuspending them is effectively nothing (as shown in
early
> testing).

Done.

https://codereview.chromium.org/2389683002/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:697: if
(!IsTabAutoDiscardable(web_contents))
On 2016/10/04 21:52:41, chrisha (slow) wrote:
> Discarding logic shouldn't affect suspending, IMO.

I see.
If needed, I will update src/chrome/common/extensions/api/tabs.json.

[tasak, 2016-10-06 08:29:24]

Description was changed from

==========
Modify PurgeAndSuspend to see WebContents

If a background renderer has some web content that matches the following
condition,
the renderer can be purged and suspended:
- no media content,
- not recently used (c.f. minimum protection time),
- not disallowed to discard automatically (reusing tab discarding logic)

BUG=635419
==========

to

==========
Modify PurgeAndSuspend to see WebContents

- If a background renderer has some web content that matches the following
condition,
the renderer can be purged and suspended:
  - no media content,

- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is

https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

[tasak, 2016-10-06 08:30:19]

Description was changed from

==========
Modify PurgeAndSuspend to see WebContents

- If a background renderer has some web content that matches the following
condition,
the renderer can be purged and suspended:
  - no media content,

- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is

https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

to

==========
Modify PurgeAndSuspend to see WebContents

- If a background renderer has no media content,
the renderer can be purged and suspended.
- intent-to-implement-and-ship of background renderer's purge + suspend is
 https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is

https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ...
- The feature is not enabled by default because purge-and-suspend-time is 0.

BUG=635419
==========

[chrisha, 2016-10-06 13:24:30]

Needs a corresponding unittest?

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

https://codereview.chromium.org/2389683002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:690: void
TabManager::PurgeAndSuspendBackgroundedTabs() {
I didn't realize this had been implemented in TabManager. The plan with MC is to
move all the throttle/suspend/discard logic to MC, so I'm curious why this ended
up here? Temporarily while MCv0 is being worked on by bashi?

[tasak, 2016-10-07 05:48:42]

Thank you for review.

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

https://codereview.chromium.org/2389683002/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.cc:690: void
TabManager::PurgeAndSuspendBackgroundedTabs() {
On 2016/10/06 13:24:30, chrisha (slow) wrote:
> I didn't realize this had been implemented in TabManager. The plan with MC is
to
> move all the throttle/suspend/discard logic to MC, so I'm curious why this
ended
> up here? Temporarily while MCv0 is being worked on by bashi?

I agree with you. The logic should be implemented in MC.
Since I don't want to disturb MCv0 work, temporarily I'm trying to implement the
logic in tab_manager.

Now bashi@ has just created a patch to move the logic to MC:
https://codereview.chromium.org/2399293002/

I'm ok to wait until bashi@'s patch is landed.

[tasak, 2016-10-07 08:17:17]

The CQ bit was checked by tasak@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 08:17:34]

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-07 08:57:26]

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

[commit-bot: I haz the power, 2016-10-07 08:57:27]

Dry run: This issue passed the CQ dry run.

[tasak, 2016-10-14 07:14:33]

Since  https://crrev.com/93362955b0109abee326322a952989fd5ee0d0f2 added
CanSuspendBackgroundedRenderer into TabManager, I will close this issue.