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)

[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;
 
 // static
 void ShellIntegration::SetAppModeInfo(const struct AppModeInfo* info) {
   gAppModeInfo = info;
 }
 
 // static
@@ skipping 97 matching lines @@
     IsInteractiveSetDefaultPermitted() {
   return false;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ShellIntegration::DefaultWebClientWorker
 //
 
 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() {
   // 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;
+  // 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);
   UpdateUI(state);
   // The worker has finished everything it needs to do, so free the observer
   // if we own it.
   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));
-}
-
 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(
     DefaultWebClientState state) {
   if (observer_) {
     switch (state) {
       case NOT_DEFAULT:
         observer_->SetDefaultWebClientUIState(STATE_NOT_DEFAULT);
         break;
       case IS_DEFAULT:
@@ skipping 10 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 // ShellIntegration::DefaultBrowserWorker
 //
 
 ShellIntegration::DefaultBrowserWorker::DefaultBrowserWorker(
     DefaultWebClientObserver* observer)
     : 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;
     case ShellIntegration::SET_DEFAULT_INTERACTIVE:
       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;
   }
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this, result));
+}
 
-  return result;
-}
+#if !defined(OS_WIN)
+void ShellIntegration::DefaultBrowserWorker::InitializeSetAsDefault() {}
+
+void ShellIntegration::DefaultBrowserWorker::FinalizeSetAsDefault(
+    bool succeeded) {}
+#endif  // !defined(OS_WIN)
 
 ///////////////////////////////////////////////////////////////////////////////
 // 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_);
+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;
 }