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

chrome/browser/shell_integration.cc

 // 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.
 
 #include "chrome/browser/shell_integration.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/prefs/pref_service.h"
@@ skipping 11 matching lines @@
 #endif
 
 #if !defined(OS_WIN)
 #include "chrome/common/channel_info.h"
 #include "chrome/grit/chromium_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #endif
 
 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();

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

is this correct? should this map _ASYNCHRONOUS to _INTERACTIVE?

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

Done.

 }
 
 static const struct ShellIntegration::AppModeInfo* gAppModeInfo = NULL;
 
 // static
 void ShellIntegration::SetAppModeInfo(const struct AppModeInfo* info) {
   gAppModeInfo = info;
 }
 
 // static
@@ skipping 80 matching lines @@
 bool ShellIntegration::SetAsDefaultProtocolClientInteractive(
     const std::string& protocol) {
   return false;
 }
 
 // static
 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() {
   return false;
 }
 
 bool ShellIntegration::DefaultWebClientObserver::
     IsInteractiveSetDefaultPermitted() {
   return false;
 }
@@ skipping 16 matching lines @@
             &DefaultWebClientWorker::ExecuteCheckIsDefault, this));
   }
 }
 
 void ShellIntegration::DefaultWebClientWorker::StartSetAsDefault() {
   bool interactive_permitted = false;
   if (observer_) {
     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,
                  interactive_permitted));
 }
 
 void ShellIntegration::DefaultWebClientWorker::ObserverDestroyed() {
   // Our associated view has gone away, so we shouldn't call back to it if
   // our worker thread returns after the view is dead.
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observer_ = NULL;

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

likewise, the timer/filter can be cancelled if the observer goes away.

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

Done.

 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultWebClientWorker, private:
 
 void ShellIntegration::DefaultWebClientWorker::ExecuteCheckIsDefault() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   DefaultWebClientState state = CheckIsDefault();
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
@@ skipping 10 matching lines @@
   if (observer_ && observer_->IsOwnedByWorker()) {
     delete observer_;
     observer_ = NULL;
   }
 }
 
 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(
     DefaultWebClientState state) {
   if (observer_) {
     switch (state) {
       case NOT_DEFAULT:
         observer_->SetDefaultWebClientUIState(STATE_NOT_DEFAULT);
         break;
       case IS_DEFAULT:
@@ skipping 18 matching lines @@
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultBrowserWorker, private:
 
 ShellIntegration::DefaultWebClientState
 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) {
+        result = ShellIntegration::SetAsDefaultBrowserInteractive();
+        BrowserThread::PostTask(
+            BrowserThread::UI, FROM_HERE,
+            base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this,
+                       result));
+      }
+      break;
+    case ShellIntegration::SET_DEFAULT_ASYNCHRONOUS:
       if (interactive_permitted)
-        result = ShellIntegration::SetAsDefaultBrowserInteractive();
+        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;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ShellIntegration::DefaultProtocolClientWorker
 //
 
 ShellIntegration::DefaultProtocolClientWorker::DefaultProtocolClientWorker(
     DefaultWebClientObserver* observer, const std::string& protocol)
     : DefaultWebClientWorker(observer),
       protocol_(protocol) {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultProtocolClientWorker, private:
 
 ShellIntegration::DefaultWebClientState
 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;
 }