Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335203003:
Add metrics to keep track of the tab activate/deactivate cycle (Closed)
|
Created:
4 years, 3 months ago by Patrick Monette Modified:
4 years, 3 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics to keep track of the tab activate/deactivate cycle
These metrics will be used to determine if the pinned state of a tab
matter when trying to determine its priority.
Committed: https://crrev.com/bd489e778e76c58320ea47184926ed6e366048cf
Cr-Commit-Position: refs/heads/master@{#420755}
Patch Set 1 #
Total comments: 42
Patch Set 2 : Addressed comments #
Total comments: 3
Patch Set 3 : Another round of comments #Patch Set 4 : Moved the TabStripModelObserver change to another CL #
Total comments: 4
Patch Set 5 : Split in 2 classes for convenient testing #Patch Set 6 : Tab discarding resistant #
Total comments: 8
Patch Set 7 : Addressed nits and fixed compilation #Patch Set 8 : Rebase #Patch Set 9 : Desktop only #Patch Set 10 : Removed a problematic dcheck #Messages
Total messages: 53 (27 generated)
pmonette@chromium.org changed reviewers: + chrisha@chromium.org
Rough version. PTAL
pmonette@chromium.org changed reviewers: + georgesak@chromium.org
https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.cc (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2016? https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:24: UMA_HISTOGRAM_COUNTS("Tab.DeactivationCount", 1); Always record this, and then have a separate specialized .Pinned one when is_pinned is true? Or would this be better named .NotPinned?
https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:24: #include "chrome/browser/metrics/tab_usage_recorder.h" nit: order. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.cc (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:35: // WebContents. It also keep tracks of the pinned state of the tab. nit: s/keep tracks/keeps track. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:36: class WebContentsData : public content::WebContentsUserData<WebContentsData> { Make this internal to TabUsageRecorder. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:61: bool is_closing_; Disallow copy. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:72: new WebContentsData(contents, is_pinned)); Use DEFINE_WEB_CONTENTS_USER_DATA_KEY so you don't need this method. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:98: : // content::WebContentsObserver(contents), nit: remove. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:128: Browser* browser_; Disallow copy. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:136: for (int i = 0; i < tab_strip_model->count(); ++i) { nit: no braces. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:183: static TabUsageRecorder* tab_usage_metrics = new TabUsageRecorder(); This should be global and its lifetime well documented. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:187: for (Browser* browser : *BrowserList::GetInstance()) { nit: no braces. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.h (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: 2016. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:16: // it records the number of time a tab is deactivated and reactivated. The nit: s/time/times. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:24: TabUsageRecorder(); Make this private? https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:25: ~TabUsageRecorder(); Add override. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:29: void OnBrowserRemoved(Browser* browser) override; Use BrowserTabStripTracker instead. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:34: std::map<Browser*, std::unique_ptr<TabStripObserver>> tab_strip_observer_map_; Comment this member. I don't see its usefulness, where is it used? https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:35: }; Disallow copy.
Take another look please. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:24: #include "chrome/browser/metrics/tab_usage_recorder.h" On 2016/09/14 16:10:16, Georges Khalil wrote: > nit: order. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.cc (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/09/14 09:57:39, chrisha (OOO til 26 Sep) wrote: > 2016? Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:24: UMA_HISTOGRAM_COUNTS("Tab.DeactivationCount", 1); On 2016/09/14 09:57:39, chrisha (OOO til 26 Sep) wrote: > Always record this, and then have a separate specialized .Pinned one when > is_pinned is true? > > Or would this be better named .NotPinned? I added a histogram suffix to get the best of both worlds. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:35: // WebContents. It also keep tracks of the pinned state of the tab. On 2016/09/14 16:10:16, Georges Khalil wrote: > nit: s/keep tracks/keeps track. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:36: class WebContentsData : public content::WebContentsUserData<WebContentsData> { On 2016/09/14 16:10:16, Georges Khalil wrote: > Make this internal to TabUsageRecorder. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:61: bool is_closing_; On 2016/09/14 16:10:17, Georges Khalil wrote: > Disallow copy. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:72: new WebContentsData(contents, is_pinned)); On 2016/09/14 16:10:16, Georges Khalil wrote: > Use DEFINE_WEB_CONTENTS_USER_DATA_KEY so you don't need this method. DEFINE_WEB_CONTENTS_USER_DATA_KEY is already used (but I moved it to the top). This function is needed because CreateForWebContents() is only defined for classes whose constructor take one parameter. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:98: : // content::WebContentsObserver(contents), On 2016/09/14 16:10:16, Georges Khalil wrote: > nit: remove. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:128: Browser* browser_; On 2016/09/14 16:10:16, Georges Khalil wrote: > Disallow copy. Removed the class. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:136: for (int i = 0; i < tab_strip_model->count(); ++i) { On 2016/09/14 16:10:16, Georges Khalil wrote: > nit: no braces. Removed this class. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:183: static TabUsageRecorder* tab_usage_metrics = new TabUsageRecorder(); On 2016/09/14 16:10:17, Georges Khalil wrote: > This should be global and its lifetime well documented. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:187: for (Browser* browser : *BrowserList::GetInstance()) { On 2016/09/14 16:10:16, Georges Khalil wrote: > nit: no braces. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.h (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/09/14 16:10:17, Georges Khalil wrote: > nit: 2016. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:16: // it records the number of time a tab is deactivated and reactivated. The On 2016/09/14 16:10:17, Georges Khalil wrote: > nit: s/time/times. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:24: TabUsageRecorder(); On 2016/09/14 16:10:17, Georges Khalil wrote: > Make this private? Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:25: ~TabUsageRecorder(); On 2016/09/14 16:10:17, Georges Khalil wrote: > Add override. Done. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:29: void OnBrowserRemoved(Browser* browser) override; On 2016/09/14 16:10:17, Georges Khalil wrote: > Use BrowserTabStripTracker instead. Done. I needed to change the TabStripModelObserver interface to make it work. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:34: std::map<Browser*, std::unique_ptr<TabStripObserver>> tab_strip_observer_map_; On 2016/09/14 16:10:17, Georges Khalil wrote: > Comment this member. I don't see its usefulness, where is it used? No longer applicable as I removed it. https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:35: }; On 2016/09/14 16:10:17, Georges Khalil wrote: > Disallow copy. Done. https://codereview.chromium.org/2335203003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (left): https://codereview.chromium.org/2335203003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:201: void TabInsertedAt(content::WebContents* contents, Cleaned up this code while I was here. An empty implementation of all function already exists in TabStripModelObserver.
Thanks for addressing earlier comments. Some more :) https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.cc (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:72: new WebContentsData(contents, is_pinned)); On 2016/09/16 00:13:59, Patrick Monette wrote: > On 2016/09/14 16:10:16, Georges Khalil wrote: > > Use DEFINE_WEB_CONTENTS_USER_DATA_KEY so you don't need this method. > > DEFINE_WEB_CONTENTS_USER_DATA_KEY is already used (but I moved it to the top). > > This function is needed because CreateForWebContents() is only defined for > classes whose constructor take one parameter. I missed DEFINE_WEB_CONTENTS_USER_DATA_KEY at the end, my bad. +1 to moving it at the top. I think this function can get confusing, regarding the expectation. The caller expects it to create the WebContentsData with the value of |is_pinned|. However, if the Data is already created, |is_pinned| is ignored, which would not be honoring the caller's request. I propose to remove this function and replace it with a GetWebContentsData which creates the WebContentsData if need be, and returns it. The caller would start by getting the WebContentsData then can modify the pinned state in a separate instruction. See an example here: https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?sq=... WDYT? https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.h (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:29: void OnBrowserRemoved(Browser* browser) override; On 2016/09/16 00:14:00, Patrick Monette wrote: > On 2016/09/14 16:10:17, Georges Khalil wrote: > > Use BrowserTabStripTracker instead. > > Done. > > I needed to change the TabStripModelObserver interface to make it work. AFAIK, the tracker should be a drop in replacement. Why is the change to the interface needed? https://codereview.chromium.org/2335203003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2335203003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:375: metrics::TabUsageRecorder::Initialize(); I think this should be in an experiment. I'm concerned about a potential regression on tab switch time, so having an experiment will give us much better control.
PTAL https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.cc (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.cc:72: new WebContentsData(contents, is_pinned)); On 2016/09/16 18:58:35, Georges Khalil wrote: > On 2016/09/16 00:13:59, Patrick Monette wrote: > > On 2016/09/14 16:10:16, Georges Khalil wrote: > > > Use DEFINE_WEB_CONTENTS_USER_DATA_KEY so you don't need this method. > > > > DEFINE_WEB_CONTENTS_USER_DATA_KEY is already used (but I moved it to the top). > > > > This function is needed because CreateForWebContents() is only defined for > > classes whose constructor take one parameter. > > I missed DEFINE_WEB_CONTENTS_USER_DATA_KEY at the end, my bad. +1 to moving it > at the top. > > I think this function can get confusing, regarding the expectation. The caller > expects it to create the WebContentsData with the value of |is_pinned|. However, > if the Data is already created, |is_pinned| is ignored, which would not be > honoring the caller's request. > > I propose to remove this function and replace it with a GetWebContentsData which > creates the WebContentsData if need be, and returns it. The caller would start > by getting the WebContentsData then can modify the pinned state in a separate > instruction. > > See an example here: > https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?sq=... > > WDYT? I addressed the confusing expectation. The change is simple enough that I didn't add a GetWebContentsData() function. Is it better? https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... File chrome/browser/metrics/tab_usage_recorder.h (right): https://codereview.chromium.org/2335203003/diff/1/chrome/browser/metrics/tab_... chrome/browser/metrics/tab_usage_recorder.h:29: void OnBrowserRemoved(Browser* browser) override; On 2016/09/16 18:58:35, Georges Khalil wrote: > On 2016/09/16 00:14:00, Patrick Monette wrote: > > On 2016/09/14 16:10:17, Georges Khalil wrote: > > > Use BrowserTabStripTracker instead. > > > > Done. > > > > I needed to change the TabStripModelObserver interface to make it work. > > AFAIK, the tracker should be a drop in replacement. Why is the change to the > interface needed? I meant adding a TabStripModel* parameter to the interface. In the last revision, when I was doing my own tracking, I was also tracking which TabStripModel is associated to which TabStripModelObserver (via tracking the Browser*). This is not something that BrowserTabStripTracker does. https://codereview.chromium.org/2335203003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2335203003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:375: metrics::TabUsageRecorder::Initialize(); On 2016/09/16 18:58:35, Georges Khalil wrote: > I think this should be in an experiment. I'm concerned about a potential > regression on tab switch time, so having an experiment will give us much better > control. IMO This is amount of prudence is too much. TabUsageRecorder does very little work and there is no reasons it should have an impact on tab switch time. But if you feel strongly about this concern, I'll add the experiment.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
pmonette@chromium.org changed reviewers: + rkaplow@chromium.org
Hey Robert, can you please take a look. Georges: Another look?
Looking good! We're just missing tests to validate that the metrics are recording the correct events. https://codereview.chromium.org/2335203003/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/tab_usage_recorder.cc (right): https://codereview.chromium.org/2335203003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/tab_usage_recorder.cc:130: WebContentsData::CreateForWebContents(contents); I think we can have news WebContentses that don't trigger TabInsertedAt, such as when reloading a discarded tab. To be safe, I suggest calling CreateForWebContents in the other events as well.
lgtm lg with description tweaks for histograms https://codereview.chromium.org/2335203003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2335203003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:61627: + The number of times a tab was deactivated, excluding closing tabs. This This isn't the "number of times" - since that implies you are storing a count here. I would treat the description as a single instance of data being stored in the histogram. I'd modify all the descriptions in this way
I added a test. Let me know what you think. https://codereview.chromium.org/2335203003/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/tab_usage_recorder.cc (right): https://codereview.chromium.org/2335203003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/tab_usage_recorder.cc:130: WebContentsData::CreateForWebContents(contents); On 2016/09/20 21:36:31, Georges Khalil wrote: > I think we can have news WebContentses that don't trigger TabInsertedAt, such as > when reloading a discarded tab. > > To be safe, I suggest calling CreateForWebContents in the other events as well. Done. https://codereview.chromium.org/2335203003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2335203003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:61627: + The number of times a tab was deactivated, excluding closing tabs. This On 2016/09/21 14:16:55, rkaplow wrote: > This isn't the "number of times" - since that implies you are storing a count > here. I would treat the description as a single instance of data being stored in > the histogram. I'd modify all the descriptions in this way Done.
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by pmonette@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #6 (id:160001) has been deleted
LGTM % nits. https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/tab_reactivation_tracker.cc (right): https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_reactivation_tracker.cc:96: if (old_contents) { nit: no braces https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_reactivation_tracker.cc:115: if (!base::ContainsKey(helper_map_, contents)) nit: braces https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/tab_reactivation_tracker.h (right): https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_reactivation_tracker.h:21: // that tab discarding disrupt the tracking and the discarded tab will be nit:s/disrupt/disrupts https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/tab_usage_recorder.h (right): https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_usage_recorder.h:16: // it records an histogram value everytime a tab is deactivated or reactivated. nit: s/an/a
Thanks! https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/tab_reactivation_tracker.cc (right): https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_reactivation_tracker.cc:96: if (old_contents) { On 2016/09/22 18:38:38, Georges Khalil wrote: > nit: no braces Done. https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_reactivation_tracker.cc:115: if (!base::ContainsKey(helper_map_, contents)) On 2016/09/22 18:38:38, Georges Khalil wrote: > nit: braces Done. https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/tab_reactivation_tracker.h (right): https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_reactivation_tracker.h:21: // that tab discarding disrupt the tracking and the discarded tab will be On 2016/09/22 18:38:38, Georges Khalil wrote: > nit:s/disrupt/disrupts Done. https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... File chrome/browser/metrics/tab_usage_recorder.h (right): https://codereview.chromium.org/2335203003/diff/180001/chrome/browser/metrics... chrome/browser/metrics/tab_usage_recorder.h:16: // it records an histogram value everytime a tab is deactivated or reactivated. On 2016/09/22 18:38:38, Georges Khalil wrote: > nit: s/an/a Done.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, georgesak@chromium.org Link to the patchset: https://codereview.chromium.org/2335203003/#ps200001 (title: "Addressed nits and fixed compilation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from georgesak@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2335203003/#ps220001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from georgesak@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2335203003/#ps240001 (title: "Desktop only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pmonette@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from georgesak@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2335203003/#ps260001 (title: "Removed a problematic dcheck")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add metrics to keep track of the tab activate/deactivate cycle These metrics will be used to determine if the pinned state of a tab matter when trying to determine its priority. ========== to ========== Add metrics to keep track of the tab activate/deactivate cycle These metrics will be used to determine if the pinned state of a tab matter when trying to determine its priority. Committed: https://crrev.com/bd489e778e76c58320ea47184926ed6e366048cf Cr-Commit-Position: refs/heads/master@{#420755} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/bd489e778e76c58320ea47184926ed6e366048cf Cr-Commit-Position: refs/heads/master@{#420755} |
