Setting chrome as the default browser is now fixed on Windows 10

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 c0e4491af74708c2b046874540e6fdb4dd300842..9e901b8274fc84208ceb64b2aadb3965516a02d7 100644
--- a/chrome/browser/shell_integration.h
+++ b/chrome/browser/shell_integration.h
@@ -11,6 +11,7 @@
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string16.h"
+#include "base/timer/timer.h"

[grt (UTC plus 2), 2015/09/28 14:31:04]

nit: remove this and instead forward decl OneShotTimer on line 20

[Patrick Monette, 2015/09/28 23:46:37]

Done.

 #include "ui/gfx/image/image_family.h"
 #include "url/gurl.h"
 
@@ -32,6 +33,20 @@ class ShellIntegration {
   // shown and this method returns true.
   static bool SetAsDefaultBrowserInteractive();
 
+  // Returns true if setting the default browser is an asynchronous operation.
+  // In practice, this is only true on Windows 10+.
+  static bool IsSetAsDefaultAsynchronous();
+
+  // Prompt the user to select the default browser by trying to open

[Peter Kasting, 2015/09/25 20:52:26]

Nit: Prompts

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+  // https://support.google.com/chrome?p=default_browser which is the "How to
+  // set Chrome as your default browser" help page. Only call this if

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Don't give the actual URL in this comment; just the description of it is
sufficient, and more future-proof.

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+  // |IsSetAsDefaultAsynchronous| is true.
+  //
+  // This call provides no indication as to whether or not the operation
+  // succeeded. Consider using DefaultBrowserWorker which will invoke its
+  // observer as appropriate.
+  static void SetAsDefaultBrowserAsynchronous();
+
   // Sets Chrome as the default client application for the given protocol
   // (only for the current user). Returns false if this operation fails.
   static bool SetAsDefaultProtocolClient(const std::string& protocol);
@@ -45,13 +60,19 @@ class ShellIntegration {
   static bool SetAsDefaultProtocolClientInteractive(
       const std::string& protocol);
 
-  // In Windows 8 a browser can be made default-in-metro only in an interactive
-  // flow. We will distinguish between two types of permissions here to avoid
-  // forcing the user into UI interaction when this should not be done.
+  // Windows 8 and Windows 10 introduced differents way to set the default

[Peter Kasting, 2015/09/25 20:52:26]

Nit: differents way -> different ways (or "a different way"?)

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+  // browser.
   enum DefaultWebClientSetPermission {
+    // The browser distribution is not permitted to be made default.

[Peter Kasting, 2015/09/25 20:52:26]

Nit: Maybe give an example of why?

     SET_DEFAULT_NOT_ALLOWED,
+    // This is the most common case where no special permission or interaction
+    // is required to set the default browser.
     SET_DEFAULT_UNATTENDED,
+    // On Windows 8, a browser can be made default only in an interactive flow.
     SET_DEFAULT_INTERACTIVE,
+    // On Windows 10+, the set as default browser flow is still interactive but
+    // is it also asynchronous.

[Peter Kasting, 2015/09/25 20:52:26]

Nit: is it -> it is

Although this might be better: "On Windows 10+, the set default browser flow is
both interactive and asynchronous."

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    SET_DEFAULT_ASYNCHRONOUS,
   };
 
   // Returns requirements for making the running browser the user's default.
@@ -226,45 +247,40 @@ class ShellIntegration {
 
     virtual ~DefaultWebClientWorker() {}
 
-   private:
-    // Function that performs the check.
-    virtual DefaultWebClientState CheckIsDefault() = 0;
-
-    // Function that sets Chrome as the default web client. Returns false if
-    // the operation fails or has been cancelled by the user.
-    virtual bool SetAsDefault(bool interactive_permitted) = 0;
-
-    // Function that handles performing the check on the file thread. This
-    // function is posted as a task onto the file thread, where it performs
-    // the check. When the check has finished the CompleteCheckIsDefault
-    // function is posted to the UI thread, where the result is sent back to
-    // the observer.
-    void ExecuteCheckIsDefault();
-
-    // Function that handles setting Chrome as the default web client on the
-    // file thread. This function is posted as a task onto the file thread.
-    // Once it is finished the CompleteSetAsDefault function is posted to the
-    // UI thread which will check the status of Chrome as the default, and
-    // send this to the observer.
-    // |interactive_permitted| indicates if the routine is allowed to carry on
-    // in context where user interaction is required (CanSetAsDefault*
-    // returns SET_DEFAULT_INTERACTIVE).
-    void ExecuteSetAsDefault(bool interactive_permitted);
-
-    // Communicate results to the observer. This function is posted as a task
-    // onto the UI thread by the ExecuteCheckIsDefault function running in the
-    // file thread.
+    // Communicates the result to the observer. In contrast to
+    // CompleteSetAsDefault, this should not be called multiple times.

[Peter Kasting, 2015/09/25 20:52:26]

Nit: Here and everywhere else in the CL you give a function name in a comment,
suffix with "()" to make it clear it's a function and not a type.

[Patrick Monette, 2015/09/28 23:46:37]

Done.

     void CompleteCheckIsDefault(DefaultWebClientState state);
 
-    // When the action to set Chrome as the default has completed this function
-    // is run. It is posted as a task back onto the UI thread by the
-    // ExecuteSetAsDefault function running in the file thread. This function
-    // will the start the check process, which, if an observer is present,
-    // reports to it the new status.
-    // |succeeded| is true if the actual call to a set-default function (from
-    // ExecuteSetAsDefault) was successful.
+    // Called when the set as default operation is finished. This then invokes
+    // FinalizeSetAsDefault and, if an observer is present, starts the check is
+    // default process. |succeeded| is true if the actual call to a set-default
+    // function was successful.
+    // It is safe to call this multiple times. Only the first call is processed
+    // after StartSetAsDefault is invoked.
     void CompleteSetAsDefault(bool succeeded);

[Peter Kasting, 2015/09/25 20:52:26]

Nit: The names "CompleteSetAsDefault()" and "FinalizeSetAsDefault()" are rather
confusingly similar.  Consider clearer and more distinct names, e.g.
"OnSetAsDefaultAttemptComplete()" and "CleanupAfterSetAsDefaultAttempt()".

[Patrick Monette, 2015/09/28 23:46:37]

Done.

 
+    // Flag that indicates if the SetAsDefault operation is in progess to
+    // prevent multiple notifications to the observer.
+    bool set_as_default_in_progress_ = false;
+
+   private:
+    // Function that performs the check. Always called on the FILE thread.

[Peter Kasting, 2015/09/25 20:52:26]

Nit: Remove "Function that"; change "Performs the check" to either "Checks
whether the browser is default", or, if that's ambiguous with another function,
something like "Helper function for XXX() that implements the actual default
browser check".

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    // Subclasses are responsible for calling CompleteCheckIsDefault on the UI
+    // thread.
+    virtual void CheckIsDefault() = 0;
+
+    // Sets Chrome as the default web client. Always called on the FILE thread.

[Peter Kasting, 2015/09/25 20:52:26]

Nit: web client -> browser?

[Patrick Monette, 2015/09/28 23:46:37]

Browser is related to http and https protocols and web client is used more
generally in the current context.

[Peter Kasting, 2015/09/28 23:57:49]

There's certainly a ton of usage of "browser" in this file and the others here. 
I've never heard the restriction that "browser" only refers to http[s] -- not
sure what you call it then when we use Chrome to look at file:, ftp:, etc. URLs.
 I've never heard the term "web client" outside your use.

In other words, I'm not sure I'm knowledgeable enough to overrule you, but
certainly none of my observation or experience supports a contention that "web
client" is clearer or more consistent than "browser" here.

[Patrick Monette, 2015/09/29 14:44:18]

Well to me it seems like the person who designed these worker classes in this
file adopted the convention of "web client" meaning the general term for
supporting a certain protocol since DefaultWebClientWorker is the abstract base
class for the 2 workers.

DefaultBrowserWorker: Specialization for http/https protocols.
DefaultProtocolClientWorker: Supports any other protocol.

It is consistent in the context of this file only.

+    // |interactive_permitted| will make SetAsDefault fails if it requires

[Peter Kasting, 2015/09/25 20:52:26]

Nit: fails -> fail

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    // interaction with the user. Subclasses are responsible for calling
+    // CompleteSetAsDefault on the UI thread.
+    virtual void SetAsDefault(bool interactive_permitted) = 0;
+
+    // Invoked on the UI thread prior to starting a SetAsDefault operation.
+    virtual void InitializeSetAsDefault() {}

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Don't give even no-op implementations of virtual functions inline; define
these in the .cc file.

[grt (UTC plus 2), 2015/09/28 14:31:04]

Just curious: I see inline empty inlines for virutal methods like this
extensively in content/public. Are those all incorrect, or are is it sometimes
okay to do so and other times not? What's the rule?

[Peter Kasting, 2015/09/28 20:40:48]

To me we shouldn't be doing that there, and it's inconsistent with the Dos And
Don'ts rules we have.  I know Blink explicitly and intentionally does inline
many empty virtual base methods, though, as that difference came up in
discussion last week.

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+
+    // Invoked on the UI thread following a SetAsDefault operation.
+    virtual void FinalizeSetAsDefault(bool succeeded) {}
+
     // Updates the UI in our associated view with the current default web
     // client state.
     void UpdateUI(DefaultWebClientState state);
@@ -280,13 +296,28 @@ class ShellIntegration {
     explicit DefaultBrowserWorker(DefaultWebClientObserver* observer);
 
    private:
-    ~DefaultBrowserWorker() override {}
+    ~DefaultBrowserWorker() override;
 
     // Check if Chrome is the default browser.
-    DefaultWebClientState CheckIsDefault() override;
+    void CheckIsDefault() override;
 
     // Set Chrome as the default browser.
-    bool SetAsDefault(bool interactive_permitted) override;
+    void SetAsDefault(bool interactive_permitted) override;
+
+    // On windows 10+, adds the default browser callback and starts the timer

[Peter Kasting, 2015/09/25 20:52:26]

Nit: Capitalize Windows (here and below)

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    // that determines if the operation was successful or not.
+    void InitializeSetAsDefault() override;
+
+    // On windows 10+, removes the default browser callback and stops the timer.
+    void FinalizeSetAsDefault(bool succeeded) override;
+
+#if defined(OS_WIN)

[Peter Kasting, 2015/09/25 20:52:26]

Nit: You can move this #if up to include the two functions above, which will
remove the need to define the no-op overrides in shell_integration.cc.

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    // Used to determine if setting the default browser was unsuccesful.
+    scoped_ptr<base::OneShotTimer> async_timer_;
+
+    // Records the time it takes to set the default browser asynchronously.
+    base::TimeTicks start_time_;
+#endif  // !defined(OS_WIN)
 
     DISALLOW_COPY_AND_ASSIGN(DefaultBrowserWorker);
   };
@@ -307,10 +338,10 @@ class ShellIntegration {
 
    private:
     // Check is Chrome is the default handler for this protocol.
-    DefaultWebClientState CheckIsDefault() override;
+    void CheckIsDefault() override;
 
     // Set Chrome as the default handler for this protocol.
-    bool SetAsDefault(bool interactive_permitted) override;
+    void SetAsDefault(bool interactive_permitted) override;
 
     std::string protocol_;