Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1426663005:
Make the histograms for setting the default browser consistent (Closed)
|
Created:
5 years, 1 month ago by Patrick Monette Modified:
5 years, 1 month ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_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. |
DescriptionMake the histograms for setting the default browser consistent
BUG=
Committed: https://crrev.com/9c1457fa00495c86d6b3017c7b5d1311ac0c2e2f
Cr-Commit-Position: refs/heads/master@{#360654}
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Comments #
Total comments: 29
Patch Set 3 : comments 2 #
Total comments: 6
Patch Set 4 : Rebasing #Patch Set 5 : Robert comments #
Total comments: 2
Patch Set 6 : Fix wrong comment #
Total comments: 4
Patch Set 7 : Sky comments #Patch Set 8 : Fixed name collision #
Messages
Total messages: 39 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
pmonette@chromium.org changed reviewers: + grt@chromium.org, rkaplow@chromium.org
Hey Robert, can you take a look at the UMA changes? grt@ take a look at everything.
looks pretty good. https://codereview.chromium.org/1426663005/diff/80001/.gitignore File .gitignore (right): https://codereview.chromium.org/1426663005/diff/80001/.gitignore#newcode435 .gitignore:435: /tools/metrics/actions/actions.old.xml ? https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:179: if (observer_) { nit: omit braces https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:240: #if defined(OS_WIN) how about doing this for all platforms? https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:280: // We don't report a success yet because we have to make sure Chrome is the nit: avoid "we" in comments. how about "Report failures here. Successes are reported in OnCheckIsDefaultComplete after checking that the change is verified." or something like that. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:283: if (!check_default_should_report_success_) { nit: omit braces https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:296: void ShellIntegration::DefaultWebClientWorker::ReportAttemptResult( this records results for both DefaultBrowserWorker and DefaultProtocolClientWorker, right? i think it makes sense to use different histograms. one way to slice it is for the subclasses to provide a histogram prefix to this class (e.g., "SetDefaultBrowser" and "SetDefaultProtocolClient"), and to compose the histogram names here. rkaplow may have better suggestions. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:301: UMA_HISTOGRAM_ENUMERATION("SetDefaultBrowser.Result", result, how about recording this for all platforms? is there a downside? https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:305: switch (result) { const base::TimeTicks delta = base::TimeTicks::Now() - start_time_; https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:308: base::TimeTicks::Now() - start_time_); delta https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:168: default_browser_worker_->StartCheckIsDefault(); huh. was this never needed? https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:154: UMA_HISTOGRAM_ENUMERATION( I'm not a fan of having an enum in ShellIntegration used by consumers to UMA why they call StartSetAsDefault. I think either one of the following is preferable (each has drawbacks): - Change StartSetAsDefault() so that it takes a value indicating why it is being invoked. This value could be the enum used to UMA within StartSetAsDefault. This has the drawback that a new value must be added to the enum each time a new callsite is introduced. - Change this so that it UMAs something like DefaultBrowser.InfoBar.StartSetAsDefault. Other callers would do similarly in their own namespace. I think the latter is good, since it is easy to view all DefaultBrowser.InfoBar.* histograms at once. https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10208: settings. ?"settings page."? https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6334: <histogram name="DefaultBrowserWarning.Ignored" enum="BooleanHit"> the following are also obsolete, right? - DefaultBrowserWarning.SetAsDefaultUI - DefaultBrowserWarning.SetAsDefault - DefaultBrowserWarning.DontSetAsDefault - DefaultBrowserWarning.SetAsDefaultUIFailed
Patchset #2 (id:100001) has been deleted
Take another look please! rkaplow@ Can you take a look at uma changes please? https://codereview.chromium.org/1426663005/diff/80001/.gitignore File .gitignore (right): https://codereview.chromium.org/1426663005/diff/80001/.gitignore#newcode435 .gitignore:435: /tools/metrics/actions/actions.old.xml On 2015/11/03 21:12:21, grt wrote: > ? There is a formatter for the file actions.xml that creates an old version instead of modifying the file directly. There is no reason to add it to git so I put it here. Its very similar to the line below (histograms.before.pretty-print.xml https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:179: if (observer_) { On 2015/11/03 21:12:21, grt wrote: > nit: omit braces Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:240: #if defined(OS_WIN) On 2015/11/03 21:12:21, grt wrote: > how about doing this for all platforms? Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:280: // We don't report a success yet because we have to make sure Chrome is the On 2015/11/03 21:12:21, grt wrote: > nit: avoid "we" in comments. how about "Report failures here. Successes are > reported in OnCheckIsDefaultComplete after checking that the change is > verified." or something like that. Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:283: if (!check_default_should_report_success_) { On 2015/11/03 21:12:21, grt wrote: > nit: omit braces Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:296: void ShellIntegration::DefaultWebClientWorker::ReportAttemptResult( On 2015/11/03 21:12:22, grt wrote: > this records results for both DefaultBrowserWorker and > DefaultProtocolClientWorker, right? i think it makes sense to use different > histograms. one way to slice it is for the subclasses to provide a histogram > prefix to this class (e.g., "SetDefaultBrowser" and "SetDefaultProtocolClient"), > and to compose the histogram names here. rkaplow may have better suggestions. Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:301: UMA_HISTOGRAM_ENUMERATION("SetDefaultBrowser.Result", result, On 2015/11/03 21:12:21, grt wrote: > how about recording this for all platforms? is there a downside? Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:305: switch (result) { On 2015/11/03 21:12:21, grt wrote: > const base::TimeTicks delta = base::TimeTicks::Now() - start_time_; Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/shell_in... chrome/browser/shell_integration.cc:308: base::TimeTicks::Now() - start_time_); On 2015/11/03 21:12:21, grt wrote: > delta Done. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller.cc (left): https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller.cc:168: default_browser_worker_->StartCheckIsDefault(); On 2015/11/03 21:12:22, grt wrote: > huh. was this never needed? StartCheckIsDefault() is already called in OnSetAsDefaultAttemptComplete. So I guess it was being called twice everytime here. https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1426663005/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt.cc:154: UMA_HISTOGRAM_ENUMERATION( On 2015/11/03 21:12:22, grt wrote: > I'm not a fan of having an enum in ShellIntegration used by consumers to UMA why > they call StartSetAsDefault. I think either one of the following is preferable > (each has drawbacks): > > - Change StartSetAsDefault() so that it takes a value indicating why it is being > invoked. This value could be the enum used to UMA within StartSetAsDefault. This > has the drawback that a new value must be added to the enum each time a new > callsite is introduced. > > - Change this so that it UMAs something like > DefaultBrowser.InfoBar.StartSetAsDefault. Other callers would do similarly in > their own namespace. > > I think the latter is good, since it is easy to view all > DefaultBrowser.InfoBar.* histograms at once. Done. https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:10208: settings. On 2015/11/03 21:12:22, grt wrote: > ?"settings page."? Done. https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1426663005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6334: <histogram name="DefaultBrowserWarning.Ignored" enum="BooleanHit"> On 2015/11/03 21:12:22, grt wrote: > the following are also obsolete, right? > - DefaultBrowserWarning.SetAsDefaultUI > - DefaultBrowserWarning.SetAsDefault > - DefaultBrowserWarning.DontSetAsDefault > - DefaultBrowserWarning.SetAsDefaultUIFailed Yes. Added.
looks good https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:70: default: leave out the "default" case so that the compiler will automatically spit out an error here if/when another value is added to the enum. https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:342: result_histogram->Add(result); nit: do away with the local variable and move "->Add(result)" to the statement above. https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:353: duration_histogram->AddTime(delta); same comment here https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.h:223: enum AttemptResult { did you make this public so that it could be seen by ShouldReportDurationForResult and AttemptResultToString? i think it's cleaner to keep it in the protected: block and make those two helpers private static functions in this class. https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.h:351: const char* GetHistogramPrefix() override { return "SetDefaultBrowser"; } move the definition of this into the .cc file (see http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...). https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6348: + SetDefaultBrowser.Result. which value? https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6399: + SetDefaultBrowser.Result. which value? https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41763: + The count of how many times users were successfully able to set Chrome as trust rkaplow with whatever he says in case he disagrees with me here: i think this should be more along the lines of "The outcome of an attempt to set Chrome as the user's default browser." similarly for SetDefaultProtocolClient below. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:75770: + Deprecated 2015/11. Renamed to AttemptResultCode2. this needs to be deprecated and a new suffix set created because the affected-histogram's name changed?
This is great you are revamping and cleaning these up. It might be nice if you added a medium length comment giving a quick rundown of the current metrics for looking at default browser state. Not sure if it makes more sense in shell_integration or in startup/default_browser.. WDYT? https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6350: <histogram name="DefaultBrowserWarning.SetAsDefaultUI" enum="BooleanHit"> there's advantages to having histograms as well. I would suggest leaving enum histrograms tracking key decision points. It's ok information being tracked by both user actions and histograms. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6317: +<histogram name="DefaultBrowser.InfoBar.Ignored" enum="BooleanHit"> for this and all histograms in this CL that are Windows only - can you mark that in the description? https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6325: +<histogram name="DefaultBrowser.InfoBar.StartSetAsDefault" enum="BooleanHit"> is there any other options on the inforbar we should be tracking through this? https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41751: +<histogram name="SetDefaultBrowser.AsyncDuration" units="milliseconds"> also, I would group all your histograms together in the same top level group. DefaultBrowser seems reasonable to me. This way people looking into these metrics will be able to look in one consistent namespace. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41763: + The count of how many times users were successfully able to set Chrome as On 2015/11/10 20:04:30, grt wrote: > trust rkaplow with whatever he says in case he disagrees with me here: i think > this should be more along the lines of "The outcome of an attempt to set Chrome > as the user's default browser." similarly for SetDefaultProtocolClient below. Agreed.
Patchset #3 (id:130001) has been deleted
Addressed comments. rkaplow@ What do you mean when you say metrics for looking at default browser state? Do you mean all the metrics that are related to setting the default browser or just the DefaultBrowser.State histogram? https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:70: default: On 2015/11/10 20:04:30, grt wrote: > leave out the "default" case so that the compiler will automatically spit out an > error here if/when another value is added to the enum. Done. https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:342: result_histogram->Add(result); On 2015/11/10 20:04:30, grt wrote: > nit: do away with the local variable and move "->Add(result)" to the statement > above. Done. https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:353: duration_histogram->AddTime(delta); On 2015/11/10 20:04:30, grt wrote: > same comment here Done. https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.h:223: enum AttemptResult { On 2015/11/10 20:04:30, grt wrote: > did you make this public so that it could be seen by > ShouldReportDurationForResult and AttemptResultToString? i think it's cleaner to > keep it in the protected: block and make those two helpers private static > functions in this class. Done. https://codereview.chromium.org/1426663005/diff/120001/chrome/browser/shell_i... chrome/browser/shell_integration.h:351: const char* GetHistogramPrefix() override { return "SetDefaultBrowser"; } On 2015/11/10 20:04:30, grt wrote: > move the definition of this into the .cc file (see > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...). Done. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6350: <histogram name="DefaultBrowserWarning.SetAsDefaultUI" enum="BooleanHit"> On 2015/11/11 15:54:00, rkaplow wrote: > there's advantages to having histograms as well. I would suggest leaving enum > histrograms tracking key decision points. > It's ok information being tracked by both user actions and histograms. Changed the comment. There is an histogram for this now. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6317: +<histogram name="DefaultBrowser.InfoBar.Ignored" enum="BooleanHit"> On 2015/11/11 15:54:00, rkaplow wrote: > for this and all histograms in this CL that are Windows only - can you mark that > in the description? Most of the histograms are now for all platforms that uses the worker. Only the DefaultBrowser.SetDefaultAsyncDuration one is windows specific. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6325: +<histogram name="DefaultBrowser.InfoBar.StartSetAsDefault" enum="BooleanHit"> On 2015/11/11 15:54:00, rkaplow wrote: > is there any other options on the inforbar we should be tracking through this? I added the "Don't ask again" button. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6348: + SetDefaultBrowser.Result. On 2015/11/10 20:04:30, grt wrote: > which value? Done. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6399: + SetDefaultBrowser.Result. On 2015/11/10 20:04:30, grt wrote: > which value? Done. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41751: +<histogram name="SetDefaultBrowser.AsyncDuration" units="milliseconds"> On 2015/11/11 15:54:00, rkaplow wrote: > also, I would group all your histograms together in the same top level group. > > DefaultBrowser seems reasonable to me. This way people looking into these > metrics will be able to look in one consistent namespace. Done. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:41763: + The count of how many times users were successfully able to set Chrome as On 2015/11/11 15:54:00, rkaplow wrote: > On 2015/11/10 20:04:30, grt wrote: > > trust rkaplow with whatever he says in case he disagrees with me here: i think > > this should be more along the lines of "The outcome of an attempt to set > Chrome > > as the user's default browser." similarly for SetDefaultProtocolClient below. > > Agreed. Done. https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:75770: + Deprecated 2015/11. Renamed to AttemptResultCode2. On 2015/11/10 20:04:30, grt wrote: > this needs to be deprecated and a new suffix set created because the > affected-histogram's name changed? Robert? Do I need to do it? I was concerned with having an obsolete histogram no longer having its histogram_suffixes in this file.
LGTM. This will make examining the stats much easier. Thanks, Patrick.
https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:75770: + Deprecated 2015/11. Renamed to AttemptResultCode2. On 2015/11/11 22:03:35, Patrick Monette wrote: > On 2015/11/10 20:04:30, grt wrote: > > this needs to be deprecated and a new suffix set created because the > > affected-histogram's name changed? > > Robert? Do I need to do it? I was concerned with having an obsolete histogram no > longer having its histogram_suffixes in this file. It should be fine, you can just leave the obsolete DefaultBrowser.SetDefaultAsyncDuration within the affected histograms here. https://codereview.chromium.org/1426663005/diff/150001/.gitignore File .gitignore (right): https://codereview.chromium.org/1426663005/diff/150001/.gitignore#newcode435 .gitignore:435: /tools/metrics/actions/actions.old.xml why is this here? https://codereview.chromium.org/1426663005/diff/150001/chrome/browser/ui/star... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1426663005/diff/150001/chrome/browser/ui/star... chrome/browser/ui/startup/default_browser_prompt.cc:154: UMA_HISTOGRAM_COUNTS("DefaultBrowser.InfoBar.StartSetAsDefault", 1); would be consistant within these histograms, probably just use BOOLEAN true https://codereview.chromium.org/1426663005/diff/150001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/150001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6317: +<histogram name="DefaultBrowser.InfoBar.DontAskAgain" enum="BooleanHit"> sorry, one more thing I'm noticing now. You have these 3 seperate histograms for tracking what happens on the Infobar. Normally in this pattern we have one histogram, with an enum for each option - this way it is easy to see, say, the % of Ignored. Is there a particular reason you wanted seperate histograms for the 3 options? Looks like historically it was this way, but looks to me like it would be cleaner putting in one histogram. Let me know if I am missing something.
Take another look robert https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:75770: + Deprecated 2015/11. Renamed to AttemptResultCode2. On 2015/11/13 15:56:14, rkaplow wrote: > On 2015/11/11 22:03:35, Patrick Monette wrote: > > On 2015/11/10 20:04:30, grt wrote: > > > this needs to be deprecated and a new suffix set created because the > > > affected-histogram's name changed? > > > > Robert? Do I need to do it? I was concerned with having an obsolete histogram > no > > longer having its histogram_suffixes in this file. > > It should be fine, you can just leave the obsolete > DefaultBrowser.SetDefaultAsyncDuration within the affected histograms here. Done. https://codereview.chromium.org/1426663005/diff/150001/.gitignore File .gitignore (right): https://codereview.chromium.org/1426663005/diff/150001/.gitignore#newcode435 .gitignore:435: /tools/metrics/actions/actions.old.xml On 2015/11/13 15:56:14, rkaplow wrote: > why is this here? extract_actions.py format the actions.xml file and backup the old file. I figured we would never want to commit the backup file (similar to the line below. histograms.before.pretty-print.xml) https://codereview.chromium.org/1426663005/diff/150001/chrome/browser/ui/star... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1426663005/diff/150001/chrome/browser/ui/star... chrome/browser/ui/startup/default_browser_prompt.cc:154: UMA_HISTOGRAM_COUNTS("DefaultBrowser.InfoBar.StartSetAsDefault", 1); On 2015/11/13 15:56:14, rkaplow wrote: > would be consistant within these histograms, probably just use BOOLEAN true Done. https://codereview.chromium.org/1426663005/diff/150001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/150001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6317: +<histogram name="DefaultBrowser.InfoBar.DontAskAgain" enum="BooleanHit"> On 2015/11/13 15:56:14, rkaplow wrote: > sorry, one more thing I'm noticing now. You have these 3 seperate histograms for > tracking what happens on the Infobar. Normally in this pattern we have one > histogram, with an enum for each option - this way it is easy to see, say, the % > of Ignored. > > Is there a particular reason you wanted seperate histograms for the 3 options? > Looks like historically it was this way, but looks to me like it would be > cleaner putting in one histogram. Let me know if I am missing something. Done.
lgtm https://codereview.chromium.org/1426663005/diff/190001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/190001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6532: + Deprecated 2015/11. Renamed to DefaultBrowser.InfoBar.Ignored. userinteraction
https://codereview.chromium.org/1426663005/diff/190001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1426663005/diff/190001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6532: + Deprecated 2015/11. Renamed to DefaultBrowser.InfoBar.Ignored. On 2015/11/16 17:38:39, rkaplow wrote: > userinteraction Done.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1426663005/#ps210001 (title: "Fix wrong comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426663005/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426663005/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
pmonette@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: PTAL Owners review required. Thanks!
On 2015/11/16 18:10:23, Patrick Monette wrote: > mailto:sky@chromium.org: PTAL Owners review required. Thanks! I'm not a good owner for all these files. What specific files do you need me to review?
I need review for these files: chrome/browser/ui/browser_command_controller.cc chrome/browser/ui/startup/default_browser_prompt.cc chrome/browser/ui/webui/options/browser_options_handler.cc chrome/browser/shell_integration.h chrome/browser/shell_integration.cc
https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:281: check_default_should_report_success_ = result == AttemptResult::SUCCESS; Is it possible to have this member communicated directly to OnCheckIsDefaultComplete so that we don't need the member? This is preferable so that it's clear how the value is propagated. https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/ui/star... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/ui/star... chrome/browser/ui/startup/default_browser_prompt.cc:52: StartSetAsDefault, START_SET_AS_DEFAULT (see style guide).
https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/shell_i... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/shell_i... chrome/browser/shell_integration.cc:281: check_default_should_report_success_ = result == AttemptResult::SUCCESS; On 2015/11/18 00:41:06, sky wrote: > Is it possible to have this member communicated directly to > OnCheckIsDefaultComplete so that we don't need the member? This is preferable so > that it's clear how the value is propagated. I agree that it could be a bit clearer but this variable is only set in one place. The simplest alternative would be to pass the value to the StartCheckIsDefault() function but since it is a public API that can be called without setting the default browser, I feel like it's worth it to keep it without a parameter. https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/ui/star... File chrome/browser/ui/startup/default_browser_prompt.cc (right): https://codereview.chromium.org/1426663005/diff/210001/chrome/browser/ui/star... chrome/browser/ui/startup/default_browser_prompt.cc:52: StartSetAsDefault, On 2015/11/18 00:41:06, sky wrote: > START_SET_AS_DEFAULT (see style guide). Done.
Ok, LGTM
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, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1426663005/#ps230001 (title: "Sky comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426663005/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426663005/230001
The CQ bit was unchecked by pmonette@chromium.org
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, sky@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1426663005/#ps250001 (title: "Fixed name collision")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426663005/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426663005/250001
Message was sent while issue was closed.
Committed patchset #8 (id:250001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9c1457fa00495c86d6b3017c7b5d1311ac0c2e2f Cr-Commit-Position: refs/heads/master@{#360654} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
