Make the histograms for setting the default browser consistent

chrome/browser/shell_integration.h

Index: chrome/browser/shell_integration.h
diff --git a/chrome/browser/shell_integration.h b/chrome/browser/shell_integration.h
index 914a08b4ba905908c2330b681c70f2fc55b07e46..1df286b0e149cb24765b9a3d10450ce3e34e5d51 100644
--- a/chrome/browser/shell_integration.h
+++ b/chrome/browser/shell_integration.h
@@ -218,28 +218,10 @@ class ShellIntegration {
   class DefaultWebClientWorker
       : public base::RefCountedThreadSafe<DefaultWebClientWorker> {
    public:
-    explicit DefaultWebClientWorker(DefaultWebClientObserver* observer);
-
-    // Checks to see if Chrome is the default web client application. The result
-    // will be passed back to the observer via the SetDefaultWebClientUIState
-    // function. If there is no observer, this function does not do anything.
-    void StartCheckIsDefault();
-
-    // Sets Chrome as the default web client application. If there is an
-    // observer, once the operation has completed the new default will be
-    // queried and the current status reported via SetDefaultWebClientUIState.
-    void StartSetAsDefault();
-
-    // Called to notify the worker that the view is gone.
-    void ObserverDestroyed();
-
-   protected:
-    friend class base::RefCountedThreadSafe<DefaultWebClientWorker>;
-
     // Possible result codes for a set-as-default operation.
     // Do not modify the ordering as it is important for UMA.
     enum AttemptResult {

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

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.

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

Done.

-      // No errors encountered.
+      // Chrome was set as the default web client.
       SUCCESS,
       // Chrome was already the default web client. This counts as a successful
       // attempt.
@@ -257,9 +239,30 @@ class ShellIntegration {
       // The user initiated another attempt while the asynchronous operation was
       // already in progress.
       RETRY,
+      // No errors were encountered yet Chrome is still not the default web
+      // client.
+      NO_ERRORS_NOT_DEFAULT,
       NUM_ATTEMPT_RESULT_TYPES
     };
 
+    explicit DefaultWebClientWorker(DefaultWebClientObserver* observer);
+
+    // Checks to see if Chrome is the default web client application. The result
+    // will be passed back to the observer via the SetDefaultWebClientUIState
+    // function. If there is no observer, this function does not do anything.
+    void StartCheckIsDefault();
+
+    // Sets Chrome as the default web client application. If there is an
+    // observer, once the operation has completed the new default will be
+    // queried and the current status reported via SetDefaultWebClientUIState.
+    void StartSetAsDefault();
+
+    // Called to notify the worker that the view is gone.
+    void ObserverDestroyed();
+
+   protected:
+    friend class base::RefCountedThreadSafe<DefaultWebClientWorker>;
+
     virtual ~DefaultWebClientWorker();
 
     // Communicates the result to the observer. In contrast to
@@ -295,6 +298,10 @@ class ShellIntegration {
     // OnSetAsDefaultAttemptComplete() on the UI thread.
     virtual void SetAsDefault(bool interactive_permitted) = 0;
 
+    // Returns the prefix used for metrics to differentiate UMA metrics for
+    // setting the default browser and setting the default protocol client.
+    virtual const char* GetHistogramPrefix() = 0;
+
     // Invoked on the UI thread prior to starting a set-as-default operation.
     // Returns true if the initialization succeeded and a subsequent call to
     // FinalizeSetAsDefault() is required.
@@ -303,9 +310,6 @@ class ShellIntegration {
     // Invoked on the UI thread following a set-as-default operation.
     virtual void FinalizeSetAsDefault();
 
-    // Returns true if the attempt results should be reported to UMA.
-    static bool ShouldReportAttemptResults();
-
     // Reports the result and duration for one set-as-default attempt.
     void ReportAttemptResult(AttemptResult result);
 
@@ -323,6 +327,10 @@ class ShellIntegration {
     // Records the time it takes to set the default browser.
     base::TimeTicks start_time_;
 
+    // Wait until Chrome has been confirmed as the default browser before
+    // reporting a successful attempt.
+    bool check_default_should_report_success_ = false;
+
     DISALLOW_COPY_AND_ASSIGN(DefaultWebClientWorker);
   };
 
@@ -340,6 +348,8 @@ class ShellIntegration {
     // Set Chrome as the default browser.
     void SetAsDefault(bool interactive_permitted) override;
 
+    const char* GetHistogramPrefix() override { return "SetDefaultBrowser"; }

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

move the definition of this into the .cc file (see
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...).

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

Done.

+
 #if defined(OS_WIN)
     // On Windows 10+, adds the default browser callback and starts the timer
     // that determines if the operation was successful or not.
@@ -355,7 +365,7 @@ class ShellIntegration {
 
     // Used to determine if setting the default browser was unsuccesful.
     scoped_ptr<base::OneShotTimer> async_timer_;
-#endif  // !defined(OS_WIN)
+#endif  // defined(OS_WIN)
 
     DISALLOW_COPY_AND_ASSIGN(DefaultBrowserWorker);
   };
@@ -381,6 +391,10 @@ class ShellIntegration {
     // Set Chrome as the default handler for this protocol.
     void SetAsDefault(bool interactive_permitted) override;
 
+    const char* GetHistogramPrefix() override {
+      return "SetDefaultProtocolClient";
+    }
+
     std::string protocol_;
 
     DISALLOW_COPY_AND_ASSIGN(DefaultProtocolClientWorker);