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

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 2ac4b5896e9b7a6900d43331a66bd853698a581f..494c556066b05820aadbe650d9830b373727a208 100644
--- a/chrome/browser/shell_integration.cc
+++ b/chrome/browser/shell_integration.cc
@@ -29,12 +29,30 @@
 
 using content::BrowserThread;
 
+#if !defined(OS_WIN)

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

Nit: Define these in the same place in the .cc file as they're listed in the .h
file.

(This file looks like it already suffers from being out of order, might be nice
to fix the existing code order first as a separate CL.)

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

Done.

+// static
+bool ShellIntegration::IsSetAsDefaultAsynchronous() {
+  return false;
+}
+
+// static
+void ShellIntegration::SetAsDefaultBrowserAsynchronous() {
+  NOTREACHED();
+}
+#endif  // !defined(OS_WIN)
+
 // static
 ShellIntegration::DefaultWebClientSetPermission
     ShellIntegration::CanSetAsDefaultProtocolClient() {
   // Allowed as long as the browser can become the operating system default
   // browser.
-  return CanSetAsDefaultBrowser();
+  auto permission = CanSetAsDefaultBrowser();

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

Nit: I think in this case using the real type is slightly more helpful:

  DefaultWebClientSetPermission permission = CanSetAsDefaultBrowser();

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

Done.

+
+  // Set as default asynchronous is only supported for default web browser.
+  if (permission == SET_DEFAULT_ASYNCHRONOUS)

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

Nit: Shorter:

  return (permission == SET_DEFAULT_ASYNCHRONOUS) ?
      SET_DEFAULT_INTERACTIVE : permission;

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

Done.

+    return SET_DEFAULT_INTERACTIVE;
+
+  return permission;
 }
 
 static const struct ShellIntegration::AppModeInfo* gAppModeInfo = NULL;
@@ -152,29 +170,34 @@ bool ShellIntegration::DefaultWebClientObserver::
 
 ShellIntegration::DefaultWebClientWorker::DefaultWebClientWorker(
     DefaultWebClientObserver* observer)
-    : observer_(observer) {
-}
+    : observer_(observer) {}
 
 void ShellIntegration::DefaultWebClientWorker::StartCheckIsDefault() {
   if (observer_) {
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(
-            &DefaultWebClientWorker::ExecuteCheckIsDefault, this));
+        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_)
+    CompleteSetAsDefault(false);
+
+  set_as_default_in_progress_ = true;
   bool interactive_permitted = false;
   if (observer_) {
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
     interactive_permitted = observer_->IsInteractiveSetDefaultPermitted();
+
+    InitializeSetAsDefault();
   }
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DefaultWebClientWorker::ExecuteSetAsDefault, this,
-                 interactive_permitted));
+
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+                          base::Bind(&DefaultWebClientWorker::SetAsDefault,
+                                     this, interactive_permitted));
 }
 
 void ShellIntegration::DefaultWebClientWorker::ObserverDestroyed() {
@@ -182,20 +205,16 @@ void ShellIntegration::DefaultWebClientWorker::ObserverDestroyed() {
   // our worker thread returns after the view is dead.
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observer_ = NULL;
+  // If set as default is currently running, CompleteSetAsDefault is invoked as

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

Nit: Avoid passive voice ("If an attempt to set the default browser is already
in progress, its result won't be posted to any observers.  Manually invoke
CompleteSetAsDefault() to ensure we free _______ [name of resource, or
whatever].")

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

Done.

+  // the result will no longer be posted to any observers and resources must be
+  // cleared.
+  if (set_as_default_in_progress_)
+    CompleteSetAsDefault(false);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultWebClientWorker, private:
 
-void ShellIntegration::DefaultWebClientWorker::ExecuteCheckIsDefault() {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  DefaultWebClientState state = CheckIsDefault();
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(
-          &DefaultWebClientWorker::CompleteCheckIsDefault, this, state));
-}
-
 void ShellIntegration::DefaultWebClientWorker::CompleteCheckIsDefault(
     DefaultWebClientState state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -208,24 +227,24 @@ void ShellIntegration::DefaultWebClientWorker::CompleteCheckIsDefault(
   }
 }
 
-void ShellIntegration::DefaultWebClientWorker::ExecuteSetAsDefault(
-    bool interactive_permitted) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-
-  bool result = SetAsDefault(interactive_permitted);
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&DefaultWebClientWorker::CompleteSetAsDefault, this, result));
-}
-
 void ShellIntegration::DefaultWebClientWorker::CompleteSetAsDefault(
     bool succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  // First tell the observer what the SetAsDefault call has returned.
-  if (observer_)
-    observer_->OnSetAsDefaultConcluded(succeeded);
-  // Set as default completed, check again to make sure it stuck...
-  StartCheckIsDefault();
+  // Hold on to a reference because if this was called via the default browser
+  // callback in StartupBrowserCreator, clearing the callback in
+  // FinalizeSetAsDefault will remove the last reference and delete the worker

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

Nit: will -> would otherwise; the worker -> us

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

Done.

+  // in the middle of the function.

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

Nit: the -> this

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

Done.

+  scoped_refptr<DefaultWebClientWorker> scoped_ref(this);
+
+  if (set_as_default_in_progress_) {
+    set_as_default_in_progress_ = false;
+
+    FinalizeSetAsDefault(succeeded);
+    if (observer_)
+      observer_->OnSetAsDefaultConcluded(succeeded);
+    // Set as default completed, check again to make sure it stuck...

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

Nit: Can you elaborate?  When would the operation "not stick"?

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

Done.

+    StartCheckIsDefault();
+  }
 }
 
 void ShellIntegration::DefaultWebClientWorker::UpdateUI(
@@ -256,18 +275,28 @@ ShellIntegration::DefaultBrowserWorker::DefaultBrowserWorker(
     : DefaultWebClientWorker(observer) {
 }
 
+ShellIntegration::DefaultBrowserWorker::~DefaultBrowserWorker() {
+  if (set_as_default_in_progress_)
+    CompleteSetAsDefault(false);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultBrowserWorker, private:
 
-ShellIntegration::DefaultWebClientState
-ShellIntegration::DefaultBrowserWorker::CheckIsDefault() {
-  return ShellIntegration::GetDefaultBrowser();
+void ShellIntegration::DefaultBrowserWorker::CheckIsDefault() {
+  DefaultWebClientState state = ShellIntegration::GetDefaultBrowser();
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultBrowserWorker::CompleteCheckIsDefault, this, state));
 }
 
-bool ShellIntegration::DefaultBrowserWorker::SetAsDefault(
+void ShellIntegration::DefaultBrowserWorker::SetAsDefault(
     bool interactive_permitted) {
   bool result = false;
   switch (ShellIntegration::CanSetAsDefaultBrowser()) {
+    case ShellIntegration::SET_DEFAULT_NOT_ALLOWED:
+      NOTREACHED();
+      break;
     case ShellIntegration::SET_DEFAULT_UNATTENDED:
       result = ShellIntegration::SetAsDefaultBrowser();
       break;
@@ -275,13 +304,27 @@ bool ShellIntegration::DefaultBrowserWorker::SetAsDefault(
       if (interactive_permitted)
         result = ShellIntegration::SetAsDefaultBrowserInteractive();
       break;
-    default:
-      NOTREACHED();
+    case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS:
+      if (interactive_permitted) {

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

Nit: If you reverse this conditional, you can omit the {} around the
now-one-line body (and it reads a little more clearly to me to not have "break;"
after a "return;").

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

Done.

+        // This function will cause CompleteSetAsDefault to be called
+        // asynchronously via a filter established in InitializeSetAsDefault().
+        ShellIntegration::SetAsDefaultBrowserAsynchronous();
+        return;
+      }
+      break;
   }
-
-  return result;
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this, result));
 }
 
+#if !defined(OS_WIN)
+void ShellIntegration::DefaultBrowserWorker::InitializeSetAsDefault() {}
+
+void ShellIntegration::DefaultBrowserWorker::FinalizeSetAsDefault(
+    bool succeeded) {}
+#endif  // !defined(OS_WIN)
+
 ///////////////////////////////////////////////////////////////////////////////
 // ShellIntegration::DefaultProtocolClientWorker
 //
@@ -295,28 +338,44 @@ ShellIntegration::DefaultProtocolClientWorker::DefaultProtocolClientWorker(
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultProtocolClientWorker, private:
 
-ShellIntegration::DefaultWebClientState
-ShellIntegration::DefaultProtocolClientWorker::CheckIsDefault() {
-  return ShellIntegration::IsDefaultProtocolClient(protocol_);
+void ShellIntegration::DefaultProtocolClientWorker::CheckIsDefault() {
+  DefaultWebClientState state =
+      ShellIntegration::IsDefaultProtocolClient(protocol_);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultProtocolClientWorker::CompleteCheckIsDefault, this,
+                 state));
 }
 
-bool ShellIntegration::DefaultProtocolClientWorker::SetAsDefault(
+void ShellIntegration::DefaultProtocolClientWorker::SetAsDefault(
     bool interactive_permitted) {
   bool result = false;
   switch (ShellIntegration::CanSetAsDefaultProtocolClient()) {
     case ShellIntegration::SET_DEFAULT_NOT_ALLOWED:
-      result = false;
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&DefaultProtocolClientWorker::CompleteSetAsDefault, this,
+                     false));
       break;
     case ShellIntegration::SET_DEFAULT_UNATTENDED:
       result = ShellIntegration::SetAsDefaultProtocolClient(protocol_);
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&DefaultProtocolClientWorker::CompleteSetAsDefault, this,
+                     result));
       break;
     case ShellIntegration::SET_DEFAULT_INTERACTIVE:
       if (interactive_permitted) {

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

Nit: If you reverse this conditional and return; instead of break;, and also
change the break; in the ASYNC case below to a return;, you can then replace all
the copied PostTask() calls in this function with a single call below the
switch.

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

Done.

-        result = ShellIntegration::SetAsDefaultProtocolClientInteractive(
-            protocol_);
+        result =
+            ShellIntegration::SetAsDefaultProtocolClientInteractive(protocol_);
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::Bind(&DefaultProtocolClientWorker::CompleteSetAsDefault, this,
+                       result));
       }
       break;
+    case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS:
+      NOTREACHED();
+      break;
   }
-
-  return result;
 }