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..67c0c4b96c7534dc5b524e0e7f824275ca41b4e2 100644
--- a/chrome/browser/shell_integration.cc
+++ b/chrome/browser/shell_integration.cc
@@ -29,6 +29,18 @@
 
 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() {
@@ -135,6 +147,13 @@ bool ShellIntegration::IsElevationNeededForSettingDefaultProtocolClient() {
   return false;
 }
 
+void ShellIntegration::DefaultBrowserWorker::InitializeSetAsDefault() {}

[grt (UTC plus 2), 2015/09/22 19:17:48]

nit: move these down immediately after
ShellIntegration::DefaultBrowserWorker::SetAsDefault to preserve the same order
as the header as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O....

[Patrick Monette, 2015/09/23 22:40:45]

Done.

+
+bool ShellIntegration::DefaultBrowserWorker::UninitializeSetAsDefault(
+    bool succeeded) {
+  return true;
+}
+
 #endif  // !defined(OS_WIN)
 
 bool ShellIntegration::DefaultWebClientObserver::IsOwnedByWorker() {
@@ -171,6 +190,9 @@ void ShellIntegration::DefaultWebClientWorker::StartSetAsDefault() {
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
     interactive_permitted = observer_->IsInteractiveSetDefaultPermitted();
   }
+
+  InitializeSetAsDefault();

[grt (UTC plus 2), 2015/09/22 19:17:48]

there's no need to register the filter or run the timer if there's no observer_,
is there?

[Patrick Monette, 2015/09/23 22:40:45]

Done.

+
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DefaultWebClientWorker::ExecuteSetAsDefault, this,
@@ -212,20 +234,24 @@ 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));
+  // This will call CompleteSetAsDefault asynchronously.
+  SetAsDefault(interactive_permitted);
 }
 
 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();
+
+  // Only notify observer is |UninitializeSetAsDefault| returns true. This is to

[grt (UTC plus 2), 2015/09/22 19:17:48]

i think it's cleaner for DefaultWebClientWorker to have its own state (e.g., a
bool) that it uses to protect against multiple calls to CompleteSetAsDefault.

also, please add a note to CompleteSetAsDefault's doc comment explaining that it
is safe for subclasses to call it multiple times, and that only the first call
is processed.

[Patrick Monette, 2015/09/23 22:40:45]

Done.

+  // filter out multiple calls to |CompleteSetAsDefault| due to possible race
+  // conditions.
+  if (UninitializeSetAsDefault(succeeded)) {
+    // 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();
+  }
 }
 
 void ShellIntegration::DefaultWebClientWorker::UpdateUI(
@@ -264,22 +290,33 @@ ShellIntegration::DefaultBrowserWorker::CheckIsDefault() {
   return ShellIntegration::GetDefaultBrowser();
 }
 
-bool ShellIntegration::DefaultBrowserWorker::SetAsDefault(
+void ShellIntegration::DefaultBrowserWorker::SetAsDefault(
     bool interactive_permitted) {
   bool result = false;
   switch (ShellIntegration::CanSetAsDefaultBrowser()) {
     case ShellIntegration::SET_DEFAULT_UNATTENDED:
       result = ShellIntegration::SetAsDefaultBrowser();
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this,
+                     result));
       break;
     case ShellIntegration::SET_DEFAULT_INTERACTIVE:
-      if (interactive_permitted)
+      if (interactive_permitted) {
         result = ShellIntegration::SetAsDefaultBrowserInteractive();
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this,
+                       result));
+      }
+      break;
+    case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS:
+      if (interactive_permitted)
+        ShellIntegration::SetAsDefaultBrowserAsynchronous();
       break;

[grt (UTC plus 2), 2015/09/22 18:21:48]

this also needs to post a call to CompleteSetAsDefault(false) for the else case,
no?

[grt (UTC plus 2), 2015/09/22 19:17:48]

maybe it's cleaner to have one PostTask at the bottom of the function, with the
one exception (SET_DEFAULT_ASYNCHRONOUS) doing an early return.

[Patrick Monette, 2015/09/23 22:40:45]

I like this suggestion. Done.

     default:
       NOTREACHED();
   }
-
-  return result;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -300,23 +337,32 @@ ShellIntegration::DefaultProtocolClientWorker::CheckIsDefault() {
   return ShellIntegration::IsDefaultProtocolClient(protocol_);
 }
 
-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(

[grt (UTC plus 2), 2015/09/22 18:21:48]

all codepaths here must call PostTask. i suggest you move it down to the bottom
of the function and just set |result| as appropriate in this switch statement.

[Patrick Monette, 2015/09/23 22:40:45]

Done.

+          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;
   }

[grt (UTC plus 2), 2015/09/22 18:21:48]

case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS:
  NOTREACHED();
  break;

[Patrick Monette, 2015/09/23 22:40:45]

Done.

-
-  return result;
 }