Make the histograms for setting the default browser consistent

chrome/browser/shell_integration.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/shell_integration.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #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"
 #include "chrome/browser/policy/policy_path_parser.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 
 #if defined(OS_CHROMEOS)
 #include "chromeos/chromeos_switches.h"
 #endif
 
 #if !defined(OS_WIN)
 #include "chrome/common/channel_info.h"
 #include "chrome/grit/chromium_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #endif
 
 using content::BrowserThread;
 
 namespace {
 
 const struct ShellIntegration::AppModeInfo* gAppModeInfo = nullptr;
 
+// Returns true if the duration should be reported to UMA for |result|.
+bool ShouldReportDurationForResult(
+    ShellIntegration::DefaultWebClientWorker::AttemptResult result) {
+  using AttemptResult = ShellIntegration::DefaultWebClientWorker::AttemptResult;
+
+  return result == AttemptResult::SUCCESS || result == AttemptResult::FAILURE ||
+         result == AttemptResult::ABANDONED || result == AttemptResult::RETRY;
+}
+
+// Returns a string based on |result| used for UMA reports.
+const char* AttemptResultToString(
+    ShellIntegration::DefaultWebClientWorker::AttemptResult result) {
+  using AttemptResult = ShellIntegration::DefaultWebClientWorker::AttemptResult;
+
+  switch (result) {
+    case AttemptResult::SUCCESS:
+      return "Success";
+    case AttemptResult::ALREADY_DEFAULT:
+      return "AlreadyDefault";
+    case AttemptResult::FAILURE:
+      return "Failure";
+    case AttemptResult::ABANDONED:
+      return "Abandoned";
+    case AttemptResult::LAUNCH_FAILURE:
+      return "LaunchFailure";
+    case AttemptResult::OTHER_WORKER:
+      return "OtherWorker";
+    case AttemptResult::RETRY:
+      return "Retry";
+    case AttemptResult::NO_ERRORS_NOT_DEFAULT:
+      return "NoErrorsNotDefault";
+    default:

[grt (UTC plus 2), 2015/11/10 20:04:30]

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.

[Patrick Monette, 2015/11/11 22:03:35]

Done.

+      NOTREACHED();
+      return "";
+  }
+}
+
 }  // namespace
 
 #if !defined(OS_WIN)
 // static
 bool ShellIntegration::SetAsDefaultBrowserInteractive() {
   return false;
 }
 
 // static
 bool ShellIntegration::IsSetAsDefaultAsynchronous() {
@@ skipping 121 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 // ShellIntegration::DefaultWebClientWorker
 //
 
 ShellIntegration::DefaultWebClientWorker::DefaultWebClientWorker(
     DefaultWebClientObserver* observer)
     : 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() {
   // Cancel the already running process if another start is requested.
   if (set_as_default_in_progress_) {
     if (set_as_default_initialized_) {
       FinalizeSetAsDefault();
       set_as_default_initialized_ = false;
     }
 
     ReportAttemptResult(AttemptResult::RETRY);
   }
 
   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();
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                           base::Bind(&DefaultWebClientWorker::SetAsDefault,
                                      this, interactive_permitted));
 }
 
 void ShellIntegration::DefaultWebClientWorker::ObserverDestroyed() {
@@ skipping 13 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultWebClientWorker, private:
 
 ShellIntegration::DefaultWebClientWorker::~DefaultWebClientWorker() {}
 
 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()) {
     delete observer_;
     observer_ = nullptr;
   }
 }
 
 void ShellIntegration::DefaultWebClientWorker::OnSetAsDefaultAttemptComplete(
     AttemptResult result) {
   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);
 
   if (set_as_default_in_progress_) {
     set_as_default_in_progress_ = false;
 
     if (set_as_default_initialized_) {
       FinalizeSetAsDefault();
       set_as_default_initialized_ = false;
     }
     if (observer_) {
       bool succeeded = result == AttemptResult::SUCCESS ||
                        result == AttemptResult::ALREADY_DEFAULT;
       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;
+    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
     // detecting that the process was successful is not 100% sure.
     // For example, on Windows 10+, the user might have unchecked the "Always
     // use this app" checkbox which can't be detected.
     StartCheckIsDefault();
   }
 }
 
 void ShellIntegration::DefaultWebClientWorker::ReportAttemptResult(
     AttemptResult result) {
-  if (!ShouldReportAttemptResults())
-    return;
+  const char* histogram_prefix = GetHistogramPrefix();
 
-  UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.AsyncSetAsDefault.Result", result,
-                            AttemptResult::NUM_ATTEMPT_RESULT_TYPES);
+  // Report result.
+  base::HistogramBase* result_histogram = base::LinearHistogram::FactoryGet(
+      base::StringPrintf("%s.Result", histogram_prefix), 1,
+      AttemptResult::NUM_ATTEMPT_RESULT_TYPES,
+      AttemptResult::NUM_ATTEMPT_RESULT_TYPES + 1,
+      base::HistogramBase::kUmaTargetedHistogramFlag);
 
-  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;
+  result_histogram->Add(result);

[grt (UTC plus 2), 2015/11/10 20:04:30]

nit: do away with the local variable and move "->Add(result)" to the statement
above.

[Patrick Monette, 2015/11/11 22:03:35]

Done.

+
+  // Report asynchronous duration.
+  if (IsSetAsDefaultAsynchronous() && ShouldReportDurationForResult(result)) {
+    base::HistogramBase* duration_histogram = base::Histogram::FactoryTimeGet(
+        base::StringPrintf("%s.AsyncDuration_%s", histogram_prefix,
+                           AttemptResultToString(result)),
+        base::TimeDelta::FromMilliseconds(10), base::TimeDelta::FromMinutes(3),
+        50, base::HistogramBase::kUmaTargetedHistogramFlag);
+
+    const base::TimeDelta delta = base::TimeTicks::Now() - start_time_;
+    duration_histogram->AddTime(delta);

[grt (UTC plus 2), 2015/11/10 20:04:30]

same comment here

[Patrick Monette, 2015/11/11 22:03:35]

Done.

   }
 }
 
 bool ShellIntegration::DefaultWebClientWorker::InitializeSetAsDefault() {
   return true;
 }
 
 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_) {
     switch (state) {
       case NOT_DEFAULT:
         observer_->SetDefaultWebClientUIState(STATE_NOT_DEFAULT);
         break;
       case IS_DEFAULT:
         observer_->SetDefaultWebClientUIState(STATE_IS_DEFAULT);
         break;
@@ skipping 112 matching lines @@
       break;
     case SET_DEFAULT_ASYNCHRONOUS:
       NOTREACHED();
       break;
   }
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&DefaultProtocolClientWorker::OnSetAsDefaultAttemptComplete,
                  this, result));
 }