Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12378025:
Record InstantExtended pref setting on startup with an UMA histogram. (Closed)
|
Created:
7 years, 9 months ago by Alexei Svitkine (slow) Modified:
7 years, 9 months ago Reviewers:
sreeram CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, gideonwald, dominich, David Black, samarth+watch_chromium.org, Jered, sreeram Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRecord InstantExtended pref setting on startup with an UMA histogram.
BUG=176689
TEST=Under InstantExtended, toggle the Instant pref on
chrome://settings, restart Chrome and check that the new
histograms show up on chrome://histograms.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188234
Patch Set 1 : #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:283: static bool recorded = false; This bool is static across the whole browser process, not per profile. So, this histogram will only be recorded for the first profile that creates a window. That's not what we want, I guess?
https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:283: static bool recorded = false; On 2013/03/01 16:42:07, sreeram wrote: > This bool is static across the whole browser process, not per profile. So, this > histogram will only be recorded for the first profile that creates a window. > That's not what we want, I guess? I agree that's kind of weird. From the bug: "It's much better to have an opt-in/opt-out/eligible-for-field-trial-to-assign-status histogram recorded regularly (either on startup or every so often or upon some user action). Then we can easily tell what percent of the population opted out. Recording a user action only tell X people opted out this day. That can zero if no one new is opting out, but we can still have a huge population that disappeared. If we don't happen to look at the user action counts on the day they disappeared then we'd be entirely oblivious to this negative reaction." Perhaps we should take this discussion there? (Note: The above quote is also kind of mixing up the about:flags setting with this one.)
On 2013/03/01 17:57:56, Alexei Svitkine wrote: > https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... > File chrome/browser/ui/search/search.cc (right): > > https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... > chrome/browser/ui/search/search.cc:283: static bool recorded = false; > On 2013/03/01 16:42:07, sreeram wrote: > > This bool is static across the whole browser process, not per profile. So, > this > > histogram will only be recorded for the first profile that creates a window. > > That's not what we want, I guess? > > I agree that's kind of weird. From the bug: > > "It's much better to have an > opt-in/opt-out/eligible-for-field-trial-to-assign-status histogram recorded > regularly (either on startup or every so often or upon some user action). Then > we can easily tell what percent of the population opted out. Recording a user > action only tell X people opted out this day. That can zero if no one new is > opting out, but we can still have a huge population that disappeared. If we > don't happen to look at the user action counts on the day they disappeared then > we'd be entirely oblivious to this negative reaction." That's beside the point. I.e., the bug is about moving from a metric (user-action) to a histogram (startup). I'm not disagreeing with that. I'm just saying that even for a startup-histogram, this CL doesn't accomplish the goal, since it only records it for one profile. Don't we want to record this for all profiles? However ... > Perhaps we should take this discussion there? ... there are indeed some issues with trying to do this (i.e., for "all profiles"), so I'll mention them on the bug.
On 2013/03/01 18:08:12, sreeram wrote: > On 2013/03/01 17:57:56, Alexei Svitkine wrote: > > > https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... > > File chrome/browser/ui/search/search.cc (right): > > > > > https://codereview.chromium.org/12378025/diff/6008/chrome/browser/ui/search/s... > > chrome/browser/ui/search/search.cc:283: static bool recorded = false; > > On 2013/03/01 16:42:07, sreeram wrote: > > > This bool is static across the whole browser process, not per profile. So, > > this > > > histogram will only be recorded for the first profile that creates a window. > > > That's not what we want, I guess? > > > > I agree that's kind of weird. From the bug: > > > > "It's much better to have an > > opt-in/opt-out/eligible-for-field-trial-to-assign-status histogram recorded > > regularly (either on startup or every so often or upon some user action). > Then > > we can easily tell what percent of the population opted out. Recording a user > > action only tell X people opted out this day. That can zero if no one new is > > opting out, but we can still have a huge population that disappeared. If we > > don't happen to look at the user action counts on the day they disappeared > then > > we'd be entirely oblivious to this negative reaction." > > That's beside the point. I.e., the bug is about moving from a metric > (user-action) to a histogram (startup). I'm not disagreeing with that. I'm just > saying that even for a startup-histogram, this CL doesn't accomplish the goal, > since it only records it for one profile. Don't we want to record this for all > profiles? > > However ... > > > Perhaps we should take this discussion there? > > ... there are indeed some issues with trying to do this (i.e., for "all > profiles"), so I'll mention them on the bug. Sreeram, are you okay with me reviving this patch per the discussion on the bug? (which would still do it on startup for the first profile launched)
On 2013/03/12 19:50:21, Alexei Svitkine wrote: > Sreeram, are you okay with me reviving this patch per the discussion on the bug? > (which would still do it on startup for the first profile launched) Yeah, I'm fine reviving this. Please rebaseline (e.g.: the IsInstantEnabled() check has moved to search.cc).
Record InstantExtended pref setting on startup with an UMA histogram. BUG=176689 TEST=Under InstantExtended, toggle the Instant pref on chrome://settings, restart Chrome and check that the new histograms show up on chrome://histograms.
rebased, PTAL
lgtm https://codereview.chromium.org/12378025/diff/34003/chrome/browser/instant/se... File chrome/browser/instant/search.cc (right): https://codereview.chromium.org/12378025/diff/34003/chrome/browser/instant/se... chrome/browser/instant/search.cc:345: static bool recorded = false; Nit: Please add a comment about 'recorded' not being profile-specific, so this histogram only gets recorded for the first profile that happens to call this code.
https://codereview.chromium.org/12378025/diff/34003/chrome/browser/instant/se... File chrome/browser/instant/search.cc (right): https://codereview.chromium.org/12378025/diff/34003/chrome/browser/instant/se... chrome/browser/instant/search.cc:345: static bool recorded = false; On 2013/03/14 18:22:44, sreeram wrote: > Nit: Please add a comment about 'recorded' not being profile-specific, so this > histogram only gets recorded for the first profile that happens to call this > code. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/12378025/44001
Message was sent while issue was closed.
Change committed as 188234 |
