New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 720131 link

Starred by 2 users
Status: Verified
Owner:
sebmarchand@chromium.org
Closed: Jan 16
Cc:
edwardjung@chromium.org
bettes@chromium.org
chrisha@chromium.org
█ rpop@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on: View detail
issue 783989
issue 800828
issue 800842


Sign in to add a comment

Add new tab count metrics

Project Member Reported by rpop@chromium.org, May 9 2017 
1. The number of tabs total at resume from sleep/hibernate
2. The number of tabs total at session restore
3. The maximum number of tabs per window (and total) that Chrome displays over the course of a day
4. State of tabstrip degradation: no truncation of title/truncation/favicons/nothing, as aggregated per day: what’s the worst state a user hit in a day?
5. Tabs that haven’t been visible and/or foreground (playing media counts) for X amount of time, splittable by total tab counts

Additional requests that I'm not prioritizing at this time are here (internal link): https://docs.google.com/document/d/1bQyeuXYHhiuKuFIsMEgolkL0UC_BO4GwNMNP06Lu18Y/edit

Comment 1 by sebmarchand@chromium.org, Jun 1 2017 

Adding this one to the list:
6. At max, how many tabs have favicons that are the same as other tabs in the same window
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2195a200b23ea42f7bfb76cc8e385037df0ead9a

commit 2195a200b23ea42f7bfb76cc8e385037df0ead9a
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Thu Jul 27 13:47:26 2017

Base for the tab stats tracker.

This is the base for the design described in this document: https://docs.google.com/a/chromium.org/document/d/1qZkPV_IG5naCgdlwmzlb8D-iAcFnJhnMqmFX8BiJ15Q/edit?usp=sharing

Bug:  720131 
Change-Id: I7fedf3e40a971f25f64b1fee420bfe27d5a0fbe7
Reviewed-on: https://chromium-review.googlesource.com/571176
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Alexei Svitkine (OOO July28-Aug6) <asvitkine@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489910}
[modify] https://crrev.com/2195a200b23ea42f7bfb76cc8e385037df0ead9a/chrome/browser/BUILD.gn
[modify] https://crrev.com/2195a200b23ea42f7bfb76cc8e385037df0ead9a/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
[add] https://crrev.com/2195a200b23ea42f7bfb76cc8e385037df0ead9a/chrome/browser/metrics/tab_stats_tracker.cc
[add] https://crrev.com/2195a200b23ea42f7bfb76cc8e385037df0ead9a/chrome/browser/metrics/tab_stats_tracker.h
[add] https://crrev.com/2195a200b23ea42f7bfb76cc8e385037df0ead9a/chrome/browser/metrics/tab_stats_tracker_unittests.cc
[modify] https://crrev.com/2195a200b23ea42f7bfb76cc8e385037df0ead9a/chrome/test/BUILD.gn
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a613a7ef943b66521d4120184c2b31a91f9866ca

commit a613a7ef943b66521d4120184c2b31a91f9866ca
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Tue Aug 01 20:36:54 2017

Stop using a LazyInstance in TabStatsTracker.

Using LazyInstance seems to be discouraged (crbug.com/686866) now that
Chrome supports Thread-safe static initialization.

Change-Id: I7649ad76ada5bd2292341f46fa1955d2ae51385e
BUG: 686866, 720131 
Reviewed-on: https://chromium-review.googlesource.com/594491
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491100}
[modify] https://crrev.com/a613a7ef943b66521d4120184c2b31a91f9866ca/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/a613a7ef943b66521d4120184c2b31a91f9866ca/chrome/browser/metrics/tab_stats_tracker.h

Comment 5 by sebmarchand@chromium.org, Aug 14 2017 

The first metric () has been added, here's some early numbers for it (Googlers only): https://uma.googleplex.com/p/chrome/histograms/?endDate=latest&dayCount=1&histograms=Tabs.NumberOfTabsOnResume&fixupData=true&showMax=true&filters=platform%2Ceq%2CW%2Cchannel%2Ceq%2C1%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

These numbers show ~15% of users with 0 tabs opened at resume, I'm trying to see if it's a bug in the code but maybe it's just an extension running in the background? (i.e. Chrome running with no window open)
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d6ff90c1f00188b332b371b374de9d62e655ceef

commit d6ff90c1f00188b332b371b374de9d62e655ceef
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Thu Aug 17 19:41:15 2017

Don't report 0 tabs on resume if Chrome is in background.

Bug:  720131 
Change-Id: I9bb9ec1c3c253ace87a5dc6341ebf1a8bc7c0bd3
Reviewed-on: https://chromium-review.googlesource.com/618789
Reviewed-by: Alexei Svitkine (very slow) <asvitkine@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495283}
[modify] https://crrev.com/d6ff90c1f00188b332b371b374de9d62e655ceef/chrome/browser/metrics/tab_stats_tracker.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 31 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ae16d1f448bf71e8f85457d87455aa8c4cc0882

commit 1ae16d1f448bf71e8f85457d87455aa8c4cc0882
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Thu Aug 31 01:34:29 2017

Tabstats: Add more metrics

This adds the following metrics:
- Tabs.MaxTabsInADay: The maximum number of tabs that Chrome displays at the same time over the
    course of a day.
- Tabs.MaxBrowsersInADay: The maximum number of browsers opened at the same time over the course of a
    day.
- Tabs.MaxTabsPerBrowserInADay: The maximum number of tabs per browser that Chrome displays over the course
    of a day.

This also add the logic to automatically report the metrics once per day (via the DailyEvent class).

Bug:  720131 
Change-Id: I4f5472c90efca2081f9f677cfa7886a376dfe426
Reviewed-on: https://chromium-review.googlesource.com/608470
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Alexei Svitkine (very slow) <asvitkine@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498711}
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/BUILD.gn
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
[add] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/metrics/tab_stats_data_store.cc
[add] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/metrics/tab_stats_data_store.h
[add] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/metrics/tab_stats_data_store_unittests.cc
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/browser/metrics/tab_stats_tracker_unittests.cc
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/chrome/test/BUILD.gn
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/components/metrics/metrics_pref_names.cc
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/components/metrics/metrics_pref_names.h
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/components/metrics/metrics_service.cc
[modify] https://crrev.com/1ae16d1f448bf71e8f85457d87455aa8c4cc0882/tools/metrics/histograms/histograms.xml
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 7 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03ba7e4ce21a7113e96d87de4dc9693cb8886b8c

commit 03ba7e4ce21a7113e96d87de4dc9693cb8886b8c
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Thu Sep 07 21:28:55 2017

Do not report the tab counts when Chrome is backgrounded.

Currently the tab count metrics get reported even if they're equal to 0,
which means that Chrome was running in background mode only since the
last time we reported the metrics. These values shouldn't be reported.

Also add a missing metric to histograms.xml.

Bug:  720131 
Change-Id: I389e3ae6401e7ce4814a8cc554011dffb56526b4
Reviewed-on: https://chromium-review.googlesource.com/650690
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500390}
[modify] https://crrev.com/03ba7e4ce21a7113e96d87de4dc9693cb8886b8c/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/03ba7e4ce21a7113e96d87de4dc9693cb8886b8c/tools/metrics/histograms/histograms.xml

Comment 9 by michae...@chromium.org, Nov 10 2017

Blockedon: 783989
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06bda9dc6e18d538fbc0dc384e80858941247168

commit 06bda9dc6e18d538fbc0dc384e80858941247168
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Mon Nov 13 20:58:20 2017

Include TabStats browser tests on CrOS

Bug:  720131 
Change-Id: Ic6e617a2ef1d9242614a157c57720242be9114c1
Reviewed-on: https://chromium-review.googlesource.com/760672
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516038}
[modify] https://crrev.com/06bda9dc6e18d538fbc0dc384e80858941247168/chrome/test/BUILD.gn
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b41fa5c40154bb29c1b13051eb6b80cb212e973a

commit b41fa5c40154bb29c1b13051eb6b80cb212e973a
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Mon Nov 13 23:30:57 2017

Move TabStatsTracker prefs out of //component/metrics

Tab metrics prefs should be registered by Chrome. Currently they live
in the metrics component itself, which isn't Chrome-specific.

The other prefs in the metrics component are related to metrics
collecting/uploading. Prefs needed for tracking arbitrary features via
metrics should be colocated with the classes that track them.

This change also ensures we don't register these prefs on Android,
where they aren't used.

Bug:  720131 
Change-Id: Ibaca7241b4dc2d62a8c631fe15a7774cd6fe474c
Reviewed-on: https://chromium-review.googlesource.com/760402
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516095}
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/browser/metrics/tab_stats_data_store.cc
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/browser/metrics/tab_stats_data_store_unittests.cc
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/browser/metrics/tab_stats_tracker_unittests.cc
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/common/pref_names.cc
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/chrome/common/pref_names.h
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/components/metrics/metrics_pref_names.cc
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/components/metrics/metrics_pref_names.h
[modify] https://crrev.com/b41fa5c40154bb29c1b13051eb6b80cb212e973a/components/metrics/metrics_service.cc
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 5 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b408349a4fc3e5d078c21d86c5d3324613c4c3da

commit b408349a4fc3e5d078c21d86c5d3324613c4c3da
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Tue Dec 05 23:25:41 2017

Split the tab_stats_tracker tests between browser_tests and unit_tests

Also moves the tab_stats_data_store tests to unit_tests as they're not
browser tests.

I'm adding some new stats to this class and having the
browser/unit_tests separation will ease the unittesting.

Bug:  720131 
Change-Id: I9a2a9c06e0968ef9b537e2eb119934590038d3f0
Reviewed-on: https://chromium-review.googlesource.com/804435
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521885}
[modify] https://crrev.com/b408349a4fc3e5d078c21d86c5d3324613c4c3da/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/b408349a4fc3e5d078c21d86c5d3324613c4c3da/chrome/browser/metrics/tab_stats_tracker.h
[add] https://crrev.com/b408349a4fc3e5d078c21d86c5d3324613c4c3da/chrome/browser/metrics/tab_stats_tracker_browsertest.cc
[modify] https://crrev.com/b408349a4fc3e5d078c21d86c5d3324613c4c3da/chrome/browser/metrics/tab_stats_tracker_unittests.cc
[modify] https://crrev.com/b408349a4fc3e5d078c21d86c5d3324613c4c3da/chrome/test/BUILD.gn
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 9 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8f00604e1012cf2ee8fc4c397570e26b46c71b3

commit e8f00604e1012cf2ee8fc4c397570e26b46c71b3
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Tue Jan 09 19:50:52 2018

Rename the tab stats tracker/datastore unittest files

Drop the 's' so the presubmit scripts treat these files as unit test
ones.

Bug:  720131 
Change-Id: I8824a257162fa46558880669d67f21e0f125ff62
Reviewed-on: https://chromium-review.googlesource.com/857351
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528080}
[rename] https://crrev.com/e8f00604e1012cf2ee8fc4c397570e26b46c71b3/chrome/browser/metrics/tab_stats_data_store_unittest.cc
[rename] https://crrev.com/e8f00604e1012cf2ee8fc4c397570e26b46c71b3/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/e8f00604e1012cf2ee8fc4c397570e26b46c71b3/chrome/test/BUILD.gn

Comment 14 by sebmarchand@chromium.org, Jan 10 2018

Blockedon: 800828

Comment 15 by sebmarchand@chromium.org, Jan 10 2018

Blockedon: 800842
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad

commit abc0fd3cc7b624fa4a245f63272646c87b6ab4ad
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Fri Jan 12 02:34:54 2018

Tracks usage of tabs over time in TabStatsTracker

This is an implementation of the solution discussed in this document:
https://docs.google.com/document/d/16NV9LX9aNlIL4HjS69qMGZ-MB8LzZpGWAxcv1lR2wJU/edit?usp=sharing

Bug:  720131 
Change-Id: I84e9c64f7c745547f8fe25e890a0ef0029ae8350
Reviewed-on: https://chromium-review.googlesource.com/674004
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528857}
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/chrome/browser/metrics/tab_stats_data_store.cc
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/chrome/browser/metrics/tab_stats_data_store.h
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/chrome/browser/metrics/tab_stats_data_store_unittest.cc
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/content/public/test/web_contents_tester.h
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/content/test/test_web_contents.cc
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/content/test/test_web_contents.h
[modify] https://crrev.com/abc0fd3cc7b624fa4a245f63272646c87b6ab4ad/tools/metrics/histograms/histograms.xml
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 15 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/535ef8607faad49560e457dec2654d9072e413ce

commit 535ef8607faad49560e457dec2654d9072e413ce
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jan 15 07:09:24 2018

Revert "Tracks usage of tabs over time in TabStatsTracker"

This reverts commit abc0fd3cc7b624fa4a245f63272646c87b6ab4ad.

Reason for revert: Seems to be causing crashes on canary: see
https://crbug.com/801887.

Original change's description:
> Tracks usage of tabs over time in TabStatsTracker
> 
> This is an implementation of the solution discussed in this document:
> https://docs.google.com/document/d/16NV9LX9aNlIL4HjS69qMGZ-MB8LzZpGWAxcv1lR2wJU/edit?usp=sharing
> 
> Bug:  720131 
> Change-Id: I84e9c64f7c745547f8fe25e890a0ef0029ae8350
> Reviewed-on: https://chromium-review.googlesource.com/674004
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528857}

TBR=sky@chromium.org,fdoray@chromium.org,asvitkine@chromium.org,sebmarchand@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  720131 
Change-Id: Idec1e6b6a6356e9c0f319688c931cf83bc4cf833
Reviewed-on: https://chromium-review.googlesource.com/866570
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529213}
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/chrome/browser/metrics/tab_stats_data_store.cc
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/chrome/browser/metrics/tab_stats_data_store.h
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/chrome/browser/metrics/tab_stats_data_store_unittest.cc
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/content/public/test/web_contents_tester.h
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/content/test/test_web_contents.cc
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/content/test/test_web_contents.h
[modify] https://crrev.com/535ef8607faad49560e457dec2654d9072e413ce/tools/metrics/histograms/histograms.xml
Project Member

Comment 18 by bugdroid1@chromium.org, Jan 15 2018

Labels: merge-merged-3321
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1630f0ab8026e159831caf6c773f54e018bf51a

commit a1630f0ab8026e159831caf6c773f54e018bf51a
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jan 15 12:07:06 2018

Revert "Tracks usage of tabs over time in TabStatsTracker"

This reverts commit abc0fd3cc7b624fa4a245f63272646c87b6ab4ad.

Reason for revert: Seems to be causing crashes on canary: see
https://crbug.com/801887.

Original change's description:
> Tracks usage of tabs over time in TabStatsTracker
> 
> This is an implementation of the solution discussed in this document:
> https://docs.google.com/document/d/16NV9LX9aNlIL4HjS69qMGZ-MB8LzZpGWAxcv1lR2wJU/edit?usp=sharing
> 
> Bug:  720131 
> Change-Id: I84e9c64f7c745547f8fe25e890a0ef0029ae8350
> Reviewed-on: https://chromium-review.googlesource.com/674004
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528857}

TBR=sky@chromium.org,fdoray@chromium.org,asvitkine@chromium.org,sebmarchand@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  720131 
Change-Id: Idec1e6b6a6356e9c0f319688c931cf83bc4cf833
Reviewed-on: https://chromium-review.googlesource.com/866570
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#529213}(cherry picked from commit 535ef8607faad49560e457dec2654d9072e413ce)
Reviewed-on: https://chromium-review.googlesource.com/865444
Reviewed-by: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/3321@{#3}
Cr-Branched-From: f79b3bae327fee2d365d8e2e3a9d8b937d86608c-refs/heads/master@{#529180}
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/chrome/browser/metrics/tab_stats_data_store.cc
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/chrome/browser/metrics/tab_stats_data_store.h
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/chrome/browser/metrics/tab_stats_data_store_unittest.cc
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/content/public/test/web_contents_tester.h
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/content/test/test_web_contents.cc
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/content/test/test_web_contents.h
[modify] https://crrev.com/a1630f0ab8026e159831caf6c773f54e018bf51a/tools/metrics/histograms/histograms.xml
Project Member

Comment 19 by bugdroid1@chromium.org, Jan 15 2018

Labels: merge-merged-3322
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19cfefa457153305bac88b2aadbc194e27949425

commit 19cfefa457153305bac88b2aadbc194e27949425
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Jan 15 12:12:04 2018

Revert "Tracks usage of tabs over time in TabStatsTracker"

This reverts commit abc0fd3cc7b624fa4a245f63272646c87b6ab4ad.

Reason for revert: Seems to be causing crashes on canary: see
https://crbug.com/801887.

Original change's description:
> Tracks usage of tabs over time in TabStatsTracker
> 
> This is an implementation of the solution discussed in this document:
> https://docs.google.com/document/d/16NV9LX9aNlIL4HjS69qMGZ-MB8LzZpGWAxcv1lR2wJU/edit?usp=sharing
> 
> Bug:  720131 
> Change-Id: I84e9c64f7c745547f8fe25e890a0ef0029ae8350
> Reviewed-on: https://chromium-review.googlesource.com/674004
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528857}

TBR=sky@chromium.org,fdoray@chromium.org,asvitkine@chromium.org,sebmarchand@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  720131 
Change-Id: Idec1e6b6a6356e9c0f319688c931cf83bc4cf833
Reviewed-on: https://chromium-review.googlesource.com/866570
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#529213}(cherry picked from commit 535ef8607faad49560e457dec2654d9072e413ce)
Reviewed-on: https://chromium-review.googlesource.com/865445
Reviewed-by: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/3322@{#3}
Cr-Branched-From: 4522efba74e35026b0f62cb1c97d793a92a0d5da-refs/heads/master@{#529187}
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/chrome/browser/metrics/tab_stats_data_store.cc
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/chrome/browser/metrics/tab_stats_data_store.h
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/chrome/browser/metrics/tab_stats_data_store_unittest.cc
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/content/public/test/web_contents_tester.h
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/content/test/test_web_contents.cc
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/content/test/test_web_contents.h
[modify] https://crrev.com/19cfefa457153305bac88b2aadbc194e27949425/tools/metrics/histograms/histograms.xml
Project Member

Comment 20 by bugdroid1@chromium.org, Jan 24 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a715bb7d30cefaa7002f028eaae578d603cfe1c

commit 9a715bb7d30cefaa7002f028eaae578d603cfe1c
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Wed Jan 24 20:53:18 2018

Update the definition of the Tabs.MaxTabsPerWindowInADay metric

The current definition ("Tabs.MaxWindowsInADay: The maximum number of
windows opened at the same time over the course of a day") is a little
bit confusing because it's not clear if this is tracking open events,or
how many windows are existing in general, this CL clarify this.

Bug:  720131 
Change-Id: Ie1cf08d7b98719a7e393b8fcd366f8ee0bea9ffb
Reviewed-on: https://chromium-review.googlesource.com/884326
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531675}
[modify] https://crrev.com/9a715bb7d30cefaa7002f028eaae578d603cfe1c/chrome/browser/metrics/tab_stats_data_store.h
[modify] https://crrev.com/9a715bb7d30cefaa7002f028eaae578d603cfe1c/tools/metrics/histograms/histograms.xml
Project Member

Comment 21 by bugdroid1@chromium.org, Feb 7 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b

commit 145fb8c9ed5eca0743522959f1a8c3f2df0fad0b
Author: Sebastien Marchand <sebmarchand@chromium.org>
Date: Wed Feb 07 04:20:46 2018

Tracks usage of tabs over time in TabStatsTracker (reland with fixes)

Reland of https://chromium-review.googlesource.com/c/chromium/src/+/674004

Patchset #1 is the original code from the previous attempt at this.

There's (at least) 2 issues in the previous attempt:
- A use-after-free caused by a misuse of a flat_map[1]:
   "existing_tabs_[new_contents] = existing_tabs_[old_contents];"
  is invalid because the references returned by operator [] on a
  flat_map are invalidated across mutations (which is the case here if
  the map has to grow because of the insertion of |new_contents|).
- A use-after-free caused by some bookkeeping issues with the
  |existing_tabs_| member in TabStatsDataStore. This is because I
  assumed that there'll be a call to
  TabStripModelObserver::TabClosingAt for every WebContents that get
  deleted, which is not the case as some WebContents might be freed
  directly.



[1] https://chromium-review.googlesource.com/c/chromium/src/+/867073/2/chrome/browser/metrics/tab_stats_data_store.cc#97


Change-Id: Id983ab0074379f8fce1b0e760e56263e2de80e07
BUG: 801887,  720131 
Reviewed-on: https://chromium-review.googlesource.com/867073
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534913}
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/chrome/browser/metrics/tab_stats_data_store.cc
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/chrome/browser/metrics/tab_stats_data_store.h
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/chrome/browser/metrics/tab_stats_data_store_unittest.cc
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/chrome/browser/metrics/tab_stats_tracker.cc
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/chrome/browser/metrics/tab_stats_tracker.h
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/chrome/browser/metrics/tab_stats_tracker_browsertest.cc
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/chrome/browser/metrics/tab_stats_tracker_unittest.cc
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/content/public/test/web_contents_tester.h
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/content/test/test_web_contents.cc
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/content/test/test_web_contents.h
[modify] https://crrev.com/145fb8c9ed5eca0743522959f1a8c3f2df0fad0b/tools/metrics/histograms/histograms.xml

Comment 22 by sebmarchand@chromium.org, Jan 16

Status: Verified (was: Assigned)
►

Sign in to add a comment