Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(114)

Issue 2275953002: Prerender: Remove deprecated prerender experiment histograms. (Closed)

Created:
4 years, 4 months ago by mattcary
Modified:
4 years, 3 months ago
Reviewers:
pasko, Ilya Sherman, mmenke
CC:
chromium-reviews, shishir+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Prerender: Remove deprecated prerender experiment histograms. This only removes the suffixing done for initial prerender experiments using the prerender mode. As a side effect, this effectively resets all prerender histograms as all current histograms currently reported are "_Enabled", and this change drops the _Enabled suffix and so renames existing histograms. However, all of those _Enabled histograms seem to be empty currently. Possibly some histograms should just be removed? BUG= Committed: https://crrev.com/81efc02ef67475e403bc5f6c4a79446eda627423 Cr-Commit-Position: refs/heads/master@{#414997}

Patch Set 1 #

Total comments: 5

Patch Set 2 : Drop histogram.xml changes #

Patch Set 3 : mor deprecations #

Total comments: 1

Patch Set 4 : don't deprecate all the histograms #

Total comments: 8

Patch Set 5 : trim a few more unsued ones #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -261 lines) Patch
M chrome/browser/predictors/autocomplete_action_predictor.cc View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/prerender/prerender_histograms.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/prerender/prerender_manager.h View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/prerender/prerender_manager.cc View 1 chunk +0 lines, -26 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 4 chunks +273 lines, -228 lines 0 comments Download

Messages

Total messages: 39 (17 generated)
mattcary
4 years, 4 months ago (2016-08-24 12:24:56 UTC) #2
pasko
thanks! https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc#newcode156 chrome/browser/prerender/prerender_histograms.cc:156: "Prerender.OmniboxPrerenderCount", 1, 2); this histogram (Prerender.OmniboxNavigationsUsedPrerenderCount_Enabled) and another ...
4 years, 4 months ago (2016-08-24 13:08:29 UTC) #5
mmenke
Thanks for doing this, LGTM (Modulo all of pasko's comments, which I agree with)
4 years, 4 months ago (2016-08-24 14:54:59 UTC) #8
mattcary
https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc#newcode156 chrome/browser/prerender/prerender_histograms.cc:156: "Prerender.OmniboxPrerenderCount", 1, 2); On 2016/08/24 13:08:29, pasko wrote: > ...
4 years, 4 months ago (2016-08-24 15:49:11 UTC) #9
pasko
https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc#newcode156 chrome/browser/prerender/prerender_histograms.cc:156: "Prerender.OmniboxPrerenderCount", 1, 2); On 2016/08/24 15:49:11, mattcary wrote: > ...
4 years, 4 months ago (2016-08-24 17:51:38 UTC) #14
mattcary
On 2016/08/24 17:51:38, pasko wrote: > https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc > File chrome/browser/prerender/prerender_histograms.cc (right): > > https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc#newcode156 > ...
4 years, 3 months ago (2016-08-25 13:43:03 UTC) #15
mattcary
On 2016/08/25 13:43:03, mattcary wrote: > On 2016/08/24 17:51:38, pasko wrote: > > > https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/prerender_histograms.cc ...
4 years, 3 months ago (2016-08-25 13:43:28 UTC) #16
pasko
https://codereview.chromium.org/2275953002/diff/40001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2275953002/diff/40001/tools/metrics/histograms/histograms.xml#newcode102573 tools/metrics/histograms/histograms.xml:102573: + <obsolete> some of the below is still not ...
4 years, 3 months ago (2016-08-25 14:05:25 UTC) #17
mattcary
Okay try #2 of X. I split out the deprecated parts of PrerenderSource to DeprecatedPrerenderSource. ...
4 years, 3 months ago (2016-08-25 16:03:28 UTC) #18
pasko
https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histograms/histograms.xml#oldcode102631 tools/metrics/histograms/histograms.xml:102631: <affected-histogram name="Prerender.FinalStatusMatchComplete"/> this guy is also deprecated, and I'm ...
4 years, 3 months ago (2016-08-25 16:32:04 UTC) #19
pasko
on the other hand, these remaining deprecated-but-not-marked-as-obsolete are not very relevant to this specific change, ...
4 years, 3 months ago (2016-08-25 17:21:33 UTC) #20
mattcary
On 2016/08/25 17:21:33, pasko wrote: > on the other hand, these remaining deprecated-but-not-marked-as-obsolete are not ...
4 years, 3 months ago (2016-08-26 15:24:14 UTC) #21
mattcary
https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histograms/histograms.xml#oldcode102631 tools/metrics/histograms/histograms.xml:102631: <affected-histogram name="Prerender.FinalStatusMatchComplete"/> On 2016/08/25 16:32:04, pasko wrote: > this ...
4 years, 3 months ago (2016-08-26 15:32:27 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2275953002/80001
4 years, 3 months ago (2016-08-26 15:32:50 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/247036)
4 years, 3 months ago (2016-08-26 15:38:56 UTC) #27
mattcary
+isherman for histograms.xml review. Thanks!
4 years, 3 months ago (2016-08-26 15:57:12 UTC) #29
Ilya Sherman
Thanks for the cleanup! LGTM.
4 years, 3 months ago (2016-08-26 21:49:48 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2275953002/80001
4 years, 3 months ago (2016-08-29 08:14:39 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/283147)
4 years, 3 months ago (2016-08-29 10:34:47 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2275953002/80001
4 years, 3 months ago (2016-08-29 10:56:47 UTC) #36
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 3 months ago (2016-08-29 11:44:35 UTC) #37
commit-bot: I haz the power
4 years, 3 months ago (2016-08-29 11:45:53 UTC) #39
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/81efc02ef67475e403bc5f6c4a79446eda627423
Cr-Commit-Position: refs/heads/master@{#414997}

Powered by Google App Engine
This is Rietveld 408576698