Make the histograms for setting the default browser consistent

chrome/browser/shell_integration.cc

Index: chrome/browser/shell_integration.cc
diff --git a/chrome/browser/shell_integration.cc b/chrome/browser/shell_integration.cc
index d8c5b8dcf38352aab3c7937c875c333fd463e3f6..c1f83461247867c57c29bcd66914515d18c8240f 100644
--- a/chrome/browser/shell_integration.cc
+++ b/chrome/browser/shell_integration.cc
@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/timer/timer.h"
@@ -176,12 +177,12 @@ ShellIntegration::DefaultWebClientWorker::DefaultWebClientWorker(
     : observer_(observer) {}
 
 void ShellIntegration::DefaultWebClientWorker::StartCheckIsDefault() {
-  if (observer_) {
+  if (observer_)
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DefaultWebClientWorker::CheckIsDefault, this));
-  }
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DefaultWebClientWorker::CheckIsDefault, this));
 }
 
 void ShellIntegration::DefaultWebClientWorker::StartSetAsDefault() {
@@ -196,15 +197,14 @@ void ShellIntegration::DefaultWebClientWorker::StartSetAsDefault() {
   }
 
   set_as_default_in_progress_ = true;
-  bool interactive_permitted = false;
+  bool interactive_permitted = true;
   if (observer_) {
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
     interactive_permitted = observer_->IsInteractiveSetDefaultPermitted();
-
-    // The initialization is only useful when there is an observer.
-    set_as_default_initialized_ = InitializeSetAsDefault();
   }
 
+  set_as_default_initialized_ = InitializeSetAsDefault();
+
   // Remember the start time.
   start_time_ = base::TimeTicks::Now();
 
@@ -237,6 +237,15 @@ void ShellIntegration::DefaultWebClientWorker::OnCheckIsDefaultComplete(
     DefaultWebClientState state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   UpdateUI(state);
+
+  if (check_default_should_report_success_) {
+    check_default_should_report_success_ = false;
+
+    ReportAttemptResult(state == DefaultWebClientState::IS_DEFAULT
+                            ? AttemptResult::SUCCESS
+                            : AttemptResult::NO_ERRORS_NOT_DEFAULT);
+  }
+
   // The worker has finished everything it needs to do, so free the observer
   // if we own it.
   if (observer_ && observer_->IsOwnedByWorker()) {
@@ -250,7 +259,7 @@ void ShellIntegration::DefaultWebClientWorker::OnSetAsDefaultAttemptComplete(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Hold on to a reference because if this was called via the default browser
   // callback in StartupBrowserCreator, clearing the callback in
-  // FinalizeSetAsDefault would otherwise remove the last reference and delete
+  // FinalizeSetAsDefault() would otherwise remove the last reference and delete
   // us in the middle of this function.
   scoped_refptr<DefaultWebClientWorker> scoped_ref(this);
 
@@ -267,7 +276,11 @@ void ShellIntegration::DefaultWebClientWorker::OnSetAsDefaultAttemptComplete(
       observer_->OnSetAsDefaultConcluded(succeeded);
     }
 
-    ReportAttemptResult(result);
+    // Report failures here. Successes are reported in
+    // OnCheckIsDefaultComplete() after checking that the change is verified.
+    check_default_should_report_success_ = result == AttemptResult::SUCCESS;

[sky, 2015/11/18 00:41:06]

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.

[Patrick Monette, 2015/11/19 17:01:04]

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.

+    if (!check_default_should_report_success_)
+      ReportAttemptResult(result);
 
     // Start the default browser check which will notify the observer as to
     // whether Chrome is really the default browser. This is needed because
@@ -280,35 +293,24 @@ void ShellIntegration::DefaultWebClientWorker::OnSetAsDefaultAttemptComplete(
 
 void ShellIntegration::DefaultWebClientWorker::ReportAttemptResult(
     AttemptResult result) {
-  if (!ShouldReportAttemptResults())
-    return;
-
-  UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.AsyncSetAsDefault.Result", result,
-                            AttemptResult::NUM_ATTEMPT_RESULT_TYPES);
-
-  switch (result) {
-    case SUCCESS:
-      UMA_HISTOGRAM_MEDIUM_TIMES(
-          "DefaultBrowser.AsyncSetAsDefault.Duration_Success",
-          base::TimeTicks::Now() - start_time_);
-      break;
-    case FAILURE:
-      UMA_HISTOGRAM_MEDIUM_TIMES(
-          "DefaultBrowser.AsyncSetAsDefault.Duration_Failure",
-          base::TimeTicks::Now() - start_time_);
-      break;
-    case ABANDONED:
-      UMA_HISTOGRAM_MEDIUM_TIMES(
-          "DefaultBrowser.AsyncSetAsDefault.Duration_Abandoned",
-          base::TimeTicks::Now() - start_time_);
-      break;
-    case RETRY:
-      UMA_HISTOGRAM_MEDIUM_TIMES(
-          "DefaultBrowser.AsyncSetAsDefault.Duration_Retry",
-          base::TimeTicks::Now() - start_time_);
-      break;
-    default:
-      break;
+  const char* histogram_prefix = GetHistogramPrefix();
+
+  // Report result.
+  base::LinearHistogram::FactoryGet(
+      base::StringPrintf("%s.SetDefaultResult", histogram_prefix), 1,
+      AttemptResult::NUM_ATTEMPT_RESULT_TYPES,
+      AttemptResult::NUM_ATTEMPT_RESULT_TYPES + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag)
+      ->Add(result);
+
+  // Report asynchronous duration.
+  if (IsSetAsDefaultAsynchronous() && ShouldReportDurationForResult(result)) {
+    base::Histogram::FactoryTimeGet(
+        base::StringPrintf("%s.SetDefaultAsyncDuration_%s", histogram_prefix,
+                           AttemptResultToString(result)),
+        base::TimeDelta::FromMilliseconds(10), base::TimeDelta::FromMinutes(3),
+        50, base::HistogramBase::kUmaTargetedHistogramFlag)
+        ->AddTime(base::TimeTicks::Now() - start_time_);
   }
 }
 
@@ -318,13 +320,6 @@ bool ShellIntegration::DefaultWebClientWorker::InitializeSetAsDefault() {
 
 void ShellIntegration::DefaultWebClientWorker::FinalizeSetAsDefault() {}
 
-#if !defined(OS_WIN)
-// static
-bool ShellIntegration::DefaultWebClientWorker::ShouldReportAttemptResults() {
-  return false;
-}
-#endif  // !defined(OS_WIN)
-
 void ShellIntegration::DefaultWebClientWorker::UpdateUI(
     DefaultWebClientState state) {
   if (observer_) {
@@ -344,6 +339,40 @@ void ShellIntegration::DefaultWebClientWorker::UpdateUI(
   }
 }
 
+// static
+bool ShellIntegration::DefaultWebClientWorker::ShouldReportDurationForResult(
+    AttemptResult result) {
+  return result == SUCCESS || result == FAILURE || result == ABANDONED ||
+         result == RETRY;
+}
+
+// static
+const char* ShellIntegration::DefaultWebClientWorker::AttemptResultToString(
+    AttemptResult result) {
+  switch (result) {
+    case SUCCESS:
+      return "Success";
+    case ALREADY_DEFAULT:
+      return "AlreadyDefault";
+    case FAILURE:
+      return "Failure";
+    case ABANDONED:
+      return "Abandoned";
+    case LAUNCH_FAILURE:
+      return "LaunchFailure";
+    case OTHER_WORKER:
+      return "OtherWorker";
+    case RETRY:
+      return "Retry";
+    case NO_ERRORS_NOT_DEFAULT:
+      return "NoErrorsNotDefault";
+    case NUM_ATTEMPT_RESULT_TYPES:
+      break;
+  }
+  NOTREACHED();
+  return "";
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // ShellIntegration::DefaultBrowserWorker
 //
@@ -408,6 +437,10 @@ void ShellIntegration::DefaultBrowserWorker::SetAsDefault(
                  result));
 }
 
+const char* ShellIntegration::DefaultBrowserWorker::GetHistogramPrefix() {
+  return "DefaultBrowser";
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // ShellIntegration::DefaultProtocolClientWorker
 //
@@ -457,3 +490,8 @@ void ShellIntegration::DefaultProtocolClientWorker::SetAsDefault(
       base::Bind(&DefaultProtocolClientWorker::OnSetAsDefaultAttemptComplete,
                  this, result));
 }
+
+const char*
+ShellIntegration::DefaultProtocolClientWorker::GetHistogramPrefix() {
+  return "DefaultProtocolClient";
+}