Make the histograms for setting the default browser consistent

chrome/browser/shell_integration.h

 // 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.
 
 #ifndef CHROME_BROWSER_SHELL_INTEGRATION_H_
 #define CHROME_BROWSER_SHELL_INTEGRATION_H_
 
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 200 matching lines @@
   };
 
   //  Helper objects that handle checking if Chrome is the default browser
   //  or application for a url protocol on Windows and Linux, and also setting
   //  it as the default. These operations are performed asynchronously on the
   //  file thread since registry access (on Windows) or the preference database
   //  (on Linux) are involved and this can be slow.
   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.
       ALREADY_DEFAULT,
       // Chrome was not set as the default web client.
       FAILURE,
       // The attempt was abandoned because the observer was destroyed.
       ABANDONED,
       // Failed to launch the process to set Chrome as the default web client
       // asynchronously.
       LAUNCH_FAILURE,
       // Another worker is already in progress to make Chrome the default web
       // client.
       OTHER_WORKER,
       // 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
     // OnSetAsDefaultAttemptComplete(), this should not be called multiple
     // times.
     void OnCheckIsDefaultComplete(DefaultWebClientState state);
 
     // Called when the set as default operation is completed. This then invokes
     // FinalizeSetAsDefault() and, if an observer is present, starts the check
     // is default process.
@@ skipping 15 matching lines @@
     // FILE thread. Subclasses are responsible for calling
     // OnCheckIsDefaultComplete() on the UI thread.
     virtual void CheckIsDefault() = 0;
 
     // Sets Chrome as the default web client. Always called on the FILE thread.
     // |interactive_permitted| will make SetAsDefault() fail if it requires
     // interaction with the user. Subclasses are responsible for calling
     // 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.
     virtual bool InitializeSetAsDefault();
 
     // 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);
 
     // Updates the UI in our associated view with the current default web
     // client state.
     void UpdateUI(DefaultWebClientState state);
 
     DefaultWebClientObserver* observer_;
 
     // Flag that indicates the return value of InitializeSetAsDefault(). If
     // true, FinalizeSetAsDefault() will be called to clear what was
     // initialized.
     bool set_as_default_initialized_ = false;
 
     // 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);
   };
 
   // Worker for checking and setting the default browser.
   class DefaultBrowserWorker : public DefaultWebClientWorker {
    public:
     explicit DefaultBrowserWorker(DefaultWebClientObserver* observer);
 
    private:
     ~DefaultBrowserWorker() override;
 
     // Check if Chrome is the default browser.
     void CheckIsDefault() override;
 
     // 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.
     bool InitializeSetAsDefault() override;
 
     // On Windows 10+, removes the default browser callback and stops the timer.
     void FinalizeSetAsDefault() override;
 
     // Prompts the user to select the default browser by trying to open the help
     // page that explains how to set Chrome as the default browser. Returns
     // false if the process needed to set Chrome default failed to launch.
     static bool SetAsDefaultBrowserAsynchronous();
 
     // 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);
   };
 
   // Worker for checking and setting the default client application
   // for a given protocol. A different worker instance is needed for each
   // protocol you are interested in, so to check or set the default for
   // multiple protocols you should use multiple worker objects.
   class DefaultProtocolClientWorker : public DefaultWebClientWorker {
    public:
     DefaultProtocolClientWorker(DefaultWebClientObserver* observer,
                                 const std::string& protocol);
 
     const std::string& protocol() const { return protocol_; }
 
    protected:
     ~DefaultProtocolClientWorker() override;
 
    private:
     // Check is Chrome is the default handler for this protocol.
     void CheckIsDefault() override;
 
     // 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);
   };
 };
 
 #endif  // CHROME_BROWSER_SHELL_INTEGRATION_H_