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..c189605c9f926f31a22d6bedd5f7ac86e48391ff 100644
--- a/chrome/browser/shell_integration.cc
+++ b/chrome/browser/shell_integration.cc
@@ -29,12 +29,30 @@
 
 using content::BrowserThread;
 
+#if !defined(OS_WIN)
+// 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();
+
+  // Set as default asynchronous is not supported for all protocols.

[grt (UTC plus 2), 2015/09/24 18:24:11]

suggestion:
"not supported for all protocols" -> "only supported for default web browser"

[Patrick Monette, 2015/09/25 20:06:24]

Done.

+  if (permission == SET_DEFAULT_ASYNCHRONOUS)
+    permission = SET_DEFAULT_INTERACTIVE;

[grt (UTC plus 2), 2015/09/24 18:24:11]

nit:
  if (blah)
    return SET_DEFAULT_INTERACTIVE;
  return permission;
or rename "permission" to "perm" and:
  return (perm == SET_DEFAULT_ASYNCHRONOUS) ? SET_DEFAULT_INTERACTIVE : perm;

[Patrick Monette, 2015/09/25 20:06:24]

Done.

+
+  return permission;
 }
 
 static const struct ShellIntegration::AppModeInfo* gAppModeInfo = NULL;
@@ -152,29 +170,30 @@ bool ShellIntegration::DefaultWebClientObserver::
 
 ShellIntegration::DefaultWebClientWorker::DefaultWebClientWorker(
     DefaultWebClientObserver* observer)
-    : observer_(observer) {
-}
+    : observer_(observer), set_as_default_completed_(true) {}
 
 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() {
+  set_as_default_completed_ = false;

[grt (UTC plus 2), 2015/09/24 18:24:11]

should this handle the case where a previous StartSetAsDefault hasn't yet
completed? maybe FinalizeSetAsDefault(false) so that the old filter callback is
removed? or have InitializeSetAsDefault() be a noop if the timer already exists?

[Patrick Monette, 2015/09/25 20:06:24]

Calling FinalizeSetAsDefault is better imo because we want to try again if the
user repeatedly click Set default browser. The intent picker might have
disappeared and we are waiting on the timer to finish.

   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 +201,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
+  // the result will no longer be posted to any observers and we want to clear
+  // up ressources.

[grt (UTC plus 2), 2015/09/24 18:24:11]

nit: resources

[Patrick Monette, 2015/09/25 20:06:24]

Done.

+  if (!set_as_default_completed_)
+    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 +223,17 @@ 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();
+  if (!set_as_default_completed_) {

[grt (UTC plus 2), 2015/09/24 18:24:11]

wdyt of set_as_default_in_progress_ instead of set_as_default_completed_ so that
it's false to begin with and true when something is happening? it makes this
conditional more clear, to boot.

[Patrick Monette, 2015/09/25 20:06:24]

Done.

+    FinalizeSetAsDefault(succeeded);
+    if (observer_)
+      observer_->OnSetAsDefaultConcluded(succeeded);
+    // Set as default completed, check again to make sure it stuck...
+    StartCheckIsDefault();
+  }
+  set_as_default_completed_ = true;

[grt (UTC plus 2), 2015/09/24 18:24:11]

nit: move this up into the body of the if()

[Patrick Monette, 2015/09/25 20:06:24]

Done.

 }
 
 void ShellIntegration::DefaultWebClientWorker::UpdateUI(
@@ -259,29 +267,49 @@ ShellIntegration::DefaultBrowserWorker::DefaultBrowserWorker(
 ///////////////////////////////////////////////////////////////////////////////
 // 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;
     case ShellIntegration::SET_DEFAULT_INTERACTIVE:
-      if (interactive_permitted)
+      if (interactive_permitted) {

[grt (UTC plus 2), 2015/09/24 18:24:11]

nit: omit braces

[Patrick Monette, 2015/09/25 20:06:24]

Done.

         result = ShellIntegration::SetAsDefaultBrowserInteractive();
+      }
+      break;
+    case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS:
+      if (interactive_permitted) {
+        // This function will cause CompleteSetAsDefault to be called
+        // asynchronously through a filter in startup_browser_creator.h.

[grt (UTC plus 2), 2015/09/24 18:24:11]

suggestion:
        // asynchronously via a filter established in InitializeSetAsDefault().

[Patrick Monette, 2015/09/25 20:06:24]

Done.

+        ShellIntegration::SetAsDefaultBrowserAsynchronous();
+        return;
+      }
       break;
-    default:
-      NOTREACHED();
   }
-
-  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 +323,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) {
-        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;
 }