Discardable property support on TabManager

Discardable property support on TabManager

BUG=621070
Committed: https://crrev.com/8665371960b29c5034562a3f65e51106c6a362b1
Cr-Commit-Position: refs/heads/master@{#407258}

[Anderson Silva, 2016-07-20 22:21:16]

Description was changed from

==========
Discardable property support on TabManager

BUG=621070
==========

to

==========
Discardable property support on TabManager

BUG=621070
==========

[Anderson Silva, 2016-07-20 22:21:16]

andersoncss@google.com changed reviewers:
+ chrisha@chromium.org, georgesak@chromium.org

[Anderson Silva, 2016-07-20 22:21:27]

ptal

[Anderson Silva, 2016-07-20 22:21:48]

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

[commit-bot: I haz the power, 2016-07-20 22:22:27]

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-07-20 23:13:32]

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

[commit-bot: I haz the power, 2016-07-20 23:13:32]

Dry run: This issue passed the CQ dry run.

[Georges Khalil, 2016-07-21 20:51:52]

First round.

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

https://codereview.chromium.org/2167843004/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.h:132:
FRIEND_TEST_ALL_PREFIXES(TabManagerObserverTest, OnDiscardableStateChange);
Add Is/Set TabDiscardable functions, and no need for friend anymore

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

https://codereview.chromium.org/2167843004/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager_web_contents_data.cc:198: // TabManager could
not exist in tests.
I don't think that's true anymore. We don't need that check.

[Anderson Silva, 2016-07-21 21:14:49]

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

https://codereview.chromium.org/2167843004/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.h:132:
FRIEND_TEST_ALL_PREFIXES(TabManagerObserverTest, OnDiscardableStateChange);
On 2016/07/21 20:51:51, Georges Khalil wrote:
> Add Is/Set TabDiscardable functions, and no need for friend anymore

Done.

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

https://codereview.chromium.org/2167843004/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager_web_contents_data.cc:198: // TabManager could
not exist in tests.
On 2016/07/21 20:51:51, Georges Khalil wrote:
> I don't think that's true anymore. We don't need that check.

Done.

[Georges Khalil, 2016-07-21 21:21:41]

Looking good.

One change throughout: Replace Discardable with AutoDiscardable.

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

https://codereview.chromium.org/2167843004/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:132: void
SetDiscardableState(content::WebContents* contents, bool state);
nit: SetTabDiscardableState (for consistency).

https://codereview.chromium.org/2167843004/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:135: bool
IsDiscardable(content::WebContents* contents);
ditto. Also, this can be const and put it before the Set (same change in cc).

[Anderson Silva, 2016-07-21 22:16:45]

cuanged to AutoDiscardable.

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

https://codereview.chromium.org/2167843004/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:132: void
SetDiscardableState(content::WebContents* contents, bool state);
On 2016/07/21 21:21:41, Georges Khalil wrote:
> nit: SetTabDiscardableState (for consistency).

Done.

https://codereview.chromium.org/2167843004/diff/20001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:135: bool
IsDiscardable(content::WebContents* contents);
On 2016/07/21 21:21:41, Georges Khalil wrote:
> ditto. Also, this can be const and put it before the Set (same change in cc).

Done.

[Anderson Silva, 2016-07-21 22:17:11]

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

[commit-bot: I haz the power, 2016-07-21 22:18:03]

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-07-21 23:50:59]

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

[commit-bot: I haz the power, 2016-07-21 23:51:00]

Dry run: This issue passed the CQ dry run.

[Georges Khalil, 2016-07-22 15:07:57]

Mostly nits, looking good.

I'll wait for Chris to sign off on this as well.

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

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:131: // Returns true if the tab is auto
discardable by auto discarding feature.
nit: reword the comment

Returns the auto discardable state of the tab. When true, the tab is eligible to
be automatically discarded when critical memory pressure hits, otherwise the tab
is ignored and will never be automatically discarded. Note that this property
doesn't block the discarding of the tab via other methods (about:discards for
instance).

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:135: void
SetTabAutoDiscardableState(content::WebContents* contents, bool state);
nit: s/AutoDiscardable/auto discardable

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:161: // WebContents
nit: line ending.
nit: s/AutoDiscardable/auto discardable

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_observer.h (right):

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_observer.h:23: // Invoked when the
AutoDiscardable state changes.
nit: s/AutoDiscardable/auto discardable

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_web_contents_data.h (right):

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.h:75: // Returns true if the
tab is auto discardable by auto discarding feature.
nit: comment change:

Returns the auto discardable state of the tab. See tab_manager.h for more
information.

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.h:108: // Is tab eligible
for auto discarding by TabManager? Defaults to true.
nit: remove "by TabManager".

[Georges Khalil, 2016-07-22 15:08:27]

On 2016/07/22 15:07:57, Georges Khalil wrote:
> Mostly nits, looking good.
> 
> I'll wait for Chris to sign off on this as well.
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> File chrome/browser/memory/tab_manager.h (right):
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> chrome/browser/memory/tab_manager.h:131: // Returns true if the tab is auto
> discardable by auto discarding feature.
> nit: reword the comment
> 
> Returns the auto discardable state of the tab. When true, the tab is eligible
to
> be automatically discarded when critical memory pressure hits, otherwise the
tab
> is ignored and will never be automatically discarded. Note that this property
> doesn't block the discarding of the tab via other methods (about:discards for
> instance).
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> chrome/browser/memory/tab_manager.h:135: void
> SetTabAutoDiscardableState(content::WebContents* contents, bool state);
> nit: s/AutoDiscardable/auto discardable
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> chrome/browser/memory/tab_manager.h:161: // WebContents
> nit: line ending.
> nit: s/AutoDiscardable/auto discardable
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> File chrome/browser/memory/tab_manager_observer.h (right):
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> chrome/browser/memory/tab_manager_observer.h:23: // Invoked when the
> AutoDiscardable state changes.
> nit: s/AutoDiscardable/auto discardable
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> File chrome/browser/memory/tab_manager_web_contents_data.h (right):
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> chrome/browser/memory/tab_manager_web_contents_data.h:75: // Returns true if
the
> tab is auto discardable by auto discarding feature.
> nit: comment change:
> 
> Returns the auto discardable state of the tab. See tab_manager.h for more
> information.
> 
>
https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
> chrome/browser/memory/tab_manager_web_contents_data.h:108: // Is tab eligible
> for auto discarding by TabManager? Defaults to true.
> nit: remove "by TabManager".

Also, we need to actually honor this new property, so a follow up CL is needed
for that.

[Anderson Silva, 2016-07-22 15:34:52]

fixed nits

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

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:131: // Returns true if the tab is auto
discardable by auto discarding feature.
On 2016/07/22 15:07:57, Georges Khalil wrote:
> nit: reword the comment
> 
> Returns the auto discardable state of the tab. When true, the tab is eligible
to
> be automatically discarded when critical memory pressure hits, otherwise the
tab
> is ignored and will never be automatically discarded. Note that this property
> doesn't block the discarding of the tab via other methods (about:discards for
> instance).

Done.

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:135: void
SetTabAutoDiscardableState(content::WebContents* contents, bool state);
On 2016/07/22 15:07:57, Georges Khalil wrote:
> nit: s/AutoDiscardable/auto discardable

Done.

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager.h:161: // WebContents
On 2016/07/22 15:07:57, Georges Khalil wrote:
> nit: line ending.
> nit: s/AutoDiscardable/auto discardable

Done.

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_observer.h (right):

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_observer.h:23: // Invoked when the
AutoDiscardable state changes.
On 2016/07/22 15:07:57, Georges Khalil wrote:
> nit: s/AutoDiscardable/auto discardable

Done.

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_web_contents_data.h (right):

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.h:75: // Returns true if the
tab is auto discardable by auto discarding feature.
On 2016/07/22 15:07:57, Georges Khalil wrote:
> nit: comment change:
> 
> Returns the auto discardable state of the tab. See tab_manager.h for more
> information.

Done.

https://codereview.chromium.org/2167843004/diff/40001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_web_contents_data.h:108: // Is tab eligible
for auto discarding by TabManager? Defaults to true.
On 2016/07/22 15:07:57, Georges Khalil wrote:
> nit: remove "by TabManager".

Done.

[Anderson Silva, 2016-07-22 16:38:26]

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

[Anderson Silva, 2016-07-22 16:38:28]

included test

[commit-bot: I haz the power, 2016-07-22 16:38:53]

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-07-22 17:15:50]

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

[commit-bot: I haz the power, 2016-07-22 17:15:51]

Dry run: This issue passed the CQ dry run.

[Georges Khalil, 2016-07-22 17:43:08]

You also need to add this property to TabStats and use it in CompareTabStats. I
would put the check right after the is_selected.

Also, test for it :)

https://codereview.chromium.org/2167843004/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_browsertest.cc (right):

https://codereview.chromium.org/2167843004/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_browsertest.cc:401:
tab_manager->minimum_protection_time_ = base::TimeDelta::FromMinutes(0);
Let's use the new function?

[Anderson Silva, 2016-07-22 18:29:25]

included auto discardable on TabStats.

https://codereview.chromium.org/2167843004/diff/80001/chrome/browser/memory/t...
File chrome/browser/memory/tab_manager_browsertest.cc (right):

https://codereview.chromium.org/2167843004/diff/80001/chrome/browser/memory/t...
chrome/browser/memory/tab_manager_browsertest.cc:401:
tab_manager->minimum_protection_time_ = base::TimeDelta::FromMinutes(0);
On 2016/07/22 17:43:08, Georges Khalil wrote:
> Let's use the new function?

The new function is in another ongoing CL (http://crrev.com/2153943002). I've
filled a bug (http://crbug.com/630644) to track this change all over the
codebase and will implement it after the other CL has landed.

[Georges Khalil, 2016-07-22 18:37:57]

lgtm % nit.

https://codereview.chromium.org/2167843004/diff/100001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager_browsertest.cc (right):

https://codereview.chromium.org/2167843004/diff/100001/chrome/browser/memory/...
chrome/browser/memory/tab_manager_browsertest.cc:425: // Shouldn't discard the
tab, since auto discardable is disactivated.
nit: s/disactivated/deactivated

[chrisha, 2016-07-22 19:16:36]

s/auto discardable/auto-discardable/ everywhere.

Otherwise, lgtm!

https://codereview.chromium.org/2167843004/diff/120001/chrome/browser/memory/...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/2167843004/diff/120001/chrome/browser/memory/...
chrome/browser/memory/tab_manager.h:131: // Returns the auto discardable state
of the tab. When true, the tab is
auto-discardable

(here and elsewhere)

[Anderson Silva, 2016-07-22 19:41:00]

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

[commit-bot: I haz the power, 2016-07-22 19:41:31]

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-07-22 20:26:00]

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

[commit-bot: I haz the power, 2016-07-22 20:26:01]

Dry run: This issue passed the CQ dry run.

[Anderson Silva, 2016-07-22 20:46:40]

The CQ bit was checked by andersoncss@google.com

[Anderson Silva, 2016-07-22 20:46:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
georgesak@chromium.org, chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2167843004/#ps140001
(title: "change to auto-discardable")

[commit-bot: I haz the power, 2016-07-22 20:47: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-07-22 20:52:13]

Description was changed from

==========
Discardable property support on TabManager

BUG=621070
==========

to

==========
Discardable property support on TabManager

BUG=621070
==========

[commit-bot: I haz the power, 2016-07-22 20:52:14]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-22 20:55:46]

Description was changed from

==========
Discardable property support on TabManager

BUG=621070
==========

to

==========
Discardable property support on TabManager

BUG=621070

Committed: https://crrev.com/8665371960b29c5034562a3f65e51106c6a362b1
Cr-Commit-Position: refs/heads/master@{#407258}
==========

[commit-bot: I haz the power, 2016-07-22 20:55:47]

Patchset 8 (id:??) landed as
https://crrev.com/8665371960b29c5034562a3f65e51106c6a362b1
Cr-Commit-Position: refs/heads/master@{#407258}