Add option to disallow the discarding of a tab that was previously discarded.

Add option to disallow the discarding of a tab that was previously discarded.

BUG=514340
Committed: https://crrev.com/ff7361126407649f2bcf483ec8789f4717043e16
Cr-Commit-Position: refs/heads/master@{#348701}

[Georges Khalil, 2015-09-10 17:59:33]

georgesak@chromium.org changed reviewers:
+ rkaplow@chromium.org, sky@chromium.org

[Georges Khalil, 2015-09-10 17:59:33]

rkaplow@chromium.org: Please review changes in histograms.xml

sky@chromium.org: Please review changes in chrome/browser/*

This allows us to get a better idea on the frequency of discarding of each tab.
Also allows us to experiment with only discarding a tab once.

[chrisha, 2015-09-10 18:07:42]

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

[chrisha, 2015-09-10 18:07:43]

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.cc:179: int nb_discards() const { return
nb_discards_; }
Ditto.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:326: int NbDiscards(int index) const;
We don't normally abbreviate things like 'Nb'.

NumberOfDiscards?
DiscardCount?

[sky, 2015-09-10 18:23:48]

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
chrome/browser/memory/oom_priority_manager.cc:188: bool
OomPriorityManager::DiscardTabById(int64 target_web_contents_id) {
You allow this to be called from about:ui. In that case why should discard_once
apply?

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
File chrome/browser/memory/oom_priority_manager.h (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
chrome/browser/memory/oom_priority_manager.h:56: void Start(bool discard_once);
Document discard_once.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:326: int NbDiscards(int index) const;
On 2015/09/10 18:07:42, chrisha wrote:
> We don't normally abbreviate things like 'Nb'.
> 
> NumberOfDiscards?
> DiscardCount?

+1
Also, this shouldn't be tracked at the TabStripModel. Track it in the
WebContents directly by way of the SupportsUserData interface.

[Georges Khalil, 2015-09-10 20:54:04]

PTAnL.

Note that I added the discard count info in about:discards to help us when
troubleshooting user issues related to discarding.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
chrome/browser/memory/oom_priority_manager.cc:188: bool
OomPriorityManager::DiscardTabById(int64 target_web_contents_id) {
On 2015/09/10 18:23:47, sky wrote:
> You allow this to be called from about:ui. In that case why should
discard_once
> apply?

The idea is not to have users influence the metrics from the wrong groups. I
guess it's a small percentage that use this feature (about:discards), but I
didn't want to take any chances.

If you prefer, I can move this up to DiscardTab() instead. This would be done
for the audio check as well.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
File chrome/browser/memory/oom_priority_manager.h (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
chrome/browser/memory/oom_priority_manager.h:56: void Start(bool discard_once);
On 2015/09/10 18:23:47, sky wrote:
> Document discard_once.

Done.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.cc:179: int nb_discards() const { return
nb_discards_; }
On 2015/09/10 18:07:42, chrisha wrote:
> Ditto.

Done.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:326: int NbDiscards(int index) const;
On 2015/09/10 18:07:42, chrisha wrote:
> We don't normally abbreviate things like 'Nb'.
> 
> NumberOfDiscards?
> DiscardCount?

Done. Used DiscardCount.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:326: int NbDiscards(int index) const;
On 2015/09/10 18:23:47, sky wrote:
> On 2015/09/10 18:07:42, chrisha wrote:
> > We don't normally abbreviate things like 'Nb'.
> > 
> > NumberOfDiscards?
> > DiscardCount?
> 
> +1
> Also, this shouldn't be tracked at the TabStripModel. Track it in the
> WebContents directly by way of the SupportsUserData interface.

Done.

Although now the discarded state and the discard count are tracked in different
places.

Should I move the discarded state to the same place in a follow up CL?

[sky, 2015-09-10 21:54:04]

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
chrome/browser/memory/oom_priority_manager.cc:188: bool
OomPriorityManager::DiscardTabById(int64 target_web_contents_id) {
On 2015/09/10 20:54:03, Georges Khalil wrote:
> On 2015/09/10 18:23:47, sky wrote:
> > You allow this to be called from about:ui. In that case why should
> discard_once
> > apply?
> 
> The idea is not to have users influence the metrics from the wrong groups. I
> guess it's a small percentage that use this feature (about:discards), but I
> didn't want to take any chances.

Isn't it already the case that someone using about:discards impacts the metrics?
Seems to me about:discards is a developer/diagnostic tool and not something I
would really worry about.

> If you prefer, I can move this up to DiscardTab() instead. This would be done
> for the audio check as well.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:326: int NbDiscards(int index) const;
On 2015/09/10 20:54:03, Georges Khalil wrote:
> On 2015/09/10 18:23:47, sky wrote:
> > On 2015/09/10 18:07:42, chrisha wrote:
> > > We don't normally abbreviate things like 'Nb'.
> > > 
> > > NumberOfDiscards?
> > > DiscardCount?
> > 
> > +1
> > Also, this shouldn't be tracked at the TabStripModel. Track it in the
> > WebContents directly by way of the SupportsUserData interface.
> 
> Done.
> 
> Although now the discarded state and the discard count are tracked in
different
> places.
> 
> Should I move the discarded state to the same place in a follow up CL?

Follow on cl is fine.

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:151: static const char kKey[];
nit: constants before constructor.

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:415: DiscardCountState*
discard_count_state = static_cast<DiscardCountState*>(
How about a static Get() in DiscardCountState that takes a WebContents and does
the static_cast? That way you don't need duplicate code on 771.

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:769: int
TabStripModel::DiscardCount(int index) const {
I don't want this in TabStripModel. Move DiscardCountState into its own .h/.cc
so anyone interested in this data can query it.

[Georges Khalil, 2015-09-11 17:39:39]

Patchset #3 (id:40001) has been deleted

[Georges Khalil, 2015-09-11 17:39:44]

Patchset #3 (id:60001) has been deleted

[Georges Khalil, 2015-09-11 17:44:31]

sky@, PTAnL.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/memory/oom_p...
chrome/browser/memory/oom_priority_manager.cc:188: bool
OomPriorityManager::DiscardTabById(int64 target_web_contents_id) {
On 2015/09/10 21:54:03, sky wrote:
> On 2015/09/10 20:54:03, Georges Khalil wrote:
> > On 2015/09/10 18:23:47, sky wrote:
> > > You allow this to be called from about:ui. In that case why should
> > discard_once
> > > apply?
> > 
> > The idea is not to have users influence the metrics from the wrong groups. I
> > guess it's a small percentage that use this feature (about:discards), but I
> > didn't want to take any chances.
> 
> Isn't it already the case that someone using about:discards impacts the
metrics?
> Seems to me about:discards is a developer/diagnostic tool and not something I
> would really worry about.
> 
> > If you prefer, I can move this up to DiscardTab() instead. This would be
done
> > for the audio check as well.
> 

Yeah makes sense.

I changed the code to only validate if a tab is playing audio or has been
previously discarded when when the discarding is not done by the user.

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/1332003002/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:326: int NbDiscards(int index) const;
On 2015/09/10 21:54:03, sky wrote:
> On 2015/09/10 20:54:03, Georges Khalil wrote:
> > On 2015/09/10 18:23:47, sky wrote:
> > > On 2015/09/10 18:07:42, chrisha wrote:
> > > > We don't normally abbreviate things like 'Nb'.
> > > > 
> > > > NumberOfDiscards?
> > > > DiscardCount?
> > > 
> > > +1
> > > Also, this shouldn't be tracked at the TabStripModel. Track it in the
> > > WebContents directly by way of the SupportsUserData interface.
> > 
> > Done.
> > 
> > Although now the discarded state and the discard count are tracked in
> different
> > places.
> > 
> > Should I move the discarded state to the same place in a follow up CL?
> 
> Follow on cl is fine.

I made the change in this CL after all. It just made more sense.

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:151: static const char kKey[];
On 2015/09/10 21:54:04, sky wrote:
> nit: constants before constructor.

Done.

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:415: DiscardCountState*
discard_count_state = static_cast<DiscardCountState*>(
On 2015/09/10 21:54:04, sky wrote:
> How about a static Get() in DiscardCountState that takes a WebContents and
does
> the static_cast? That way you don't need duplicate code on 771.

Good idea, done.

https://codereview.chromium.org/1332003002/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:769: int
TabStripModel::DiscardCount(int index) const {
On 2015/09/10 21:54:04, sky wrote:
> I don't want this in TabStripModel. Move DiscardCountState into its own .h/.cc
> so anyone interested in this data can query it.

Done. Although I am not sure if it's in the correct subfolder.

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.cc:184:
DiscardTabById(least_important_tab_id))
There's duplicate work done in CanDiscardTab & DiscardTabById but I can't find
an elegant way of eliminating that because DiscardTabById is called in
about_ui.cc and we can't really change the parameters.

[sky, 2015-09-11 20:00:35]

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.cc:464: bool
OomPriorityManager::CanDiscardTab(int64 target_web_contents_id) const {
nit: make position match that of header.

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_discard_state.h (right):

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_discard_state.h:18: static const char kKey[];
This doesn't need to be public, does it? In fact I think it can exist entirely
in the .cc.

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:1331:
TabDiscardState::SetDiscardState(new_contents, false);
Is this line necessary? DOesn't 255 cover it?

[rkaplow, 2015-09-11 21:07:03]

https://codereview.chromium.org/1332003002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1332003002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1171:
g_browser_process->GetOomPriorityManager()->Start(group_name ==
suggest using StartsWith

https://codereview.chromium.org/1332003002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1332003002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:46995: +    bin (1, 2, 3...).
this is a strange way of recording it. Why not just increment every time there's
a discard? With this the sum / mean / etc doesn't make sense.

[Georges Khalil, 2015-09-14 18:14:47]

Patchset #5 (id:120001) has been deleted

[Georges Khalil, 2015-09-14 18:17:49]

sky@, rkaplow@, PTAnL.

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/memory/o...
File chrome/browser/memory/oom_priority_manager.cc (right):

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/memory/o...
chrome/browser/memory/oom_priority_manager.cc:464: bool
OomPriorityManager::CanDiscardTab(int64 target_web_contents_id) const {
On 2015/09/11 20:00:35, sky wrote:
> nit: make position match that of header.

There are multiple issues with the function orders in this module, not just
because of this CL.

I can do another CL right after that fixes all the ordering, so as to not
clutter this one. WDYT?

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_discard_state.h (right):

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_discard_state.h:18: static const char kKey[];
On 2015/09/11 20:00:35, sky wrote:
> This doesn't need to be public, does it? In fact I think it can exist entirely
> in the .cc.

True, done.

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_model.cc (right):

https://codereview.chromium.org/1332003002/diff/80001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_model.cc:1331:
TabDiscardState::SetDiscardState(new_contents, false);
On 2015/09/11 20:00:35, sky wrote:
> Is this line necessary? DOesn't 255 cover it?

Without this, the unit test will fail, as DidStartLoading never gets executed.

But you just brought up a big issue here! This metric is screwed up badly.
Basically We have a race issue now, depending on which event gets fired first
(DidStartLoading or NotifyIfActiveTabChanged). Most of the tab switching to
discarded tabs are being recorded as a switch to loaded tabs, because the
discarded flag is being cleared before we hit this. Unfortunately, we have been
using this metric for a while :(

I am removing these metrics for now and will reimplement them properly in a
separate CL.

https://codereview.chromium.org/1332003002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1332003002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1171:
g_browser_process->GetOomPriorityManager()->Start(group_name ==
On 2015/09/11 21:07:03, rkaplow wrote:
> suggest using StartsWith

Done.

https://codereview.chromium.org/1332003002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1332003002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:46995: +    bin (1, 2, 3...).
On 2015/09/11 21:07:03, rkaplow wrote:
> this is a strange way of recording it. Why not just increment every time
there's
> a discard? With this the sum / mean / etc doesn't make sense.

Discussed offline, keeping as is.

[sky, 2015-09-14 19:19:59]

LGTM

[rkaplow, 2015-09-14 20:08:59]

lgtm

[Georges Khalil, 2015-09-14 20:09:22]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2015-09-14 20:11:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332003002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332003002/140001

[commit-bot: I haz the power, 2015-09-14 20:17:31]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-09-14 20:18:05]

Patchset 5 (id:??) landed as
https://crrev.com/ff7361126407649f2bcf483ec8789f4717043e16
Cr-Commit-Position: refs/heads/master@{#348701}

[commit-bot: I haz the power, 2015-09-23 12:36:33]

Patchset 5 (id:??) landed as
https://crrev.com/ff7361126407649f2bcf483ec8789f4717043e16
Cr-Commit-Position: refs/heads/master@{#348701}