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..2f21fba0889af6fe95ec653449389236e58859db 100644
--- a/chrome/browser/shell_integration.cc
+++ b/chrome/browser/shell_integration.cc
@@ -7,10 +7,12 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/policy/policy_path_parser.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -34,7 +36,11 @@ ShellIntegration::DefaultWebClientSetPermission
     ShellIntegration::CanSetAsDefaultProtocolClient() {
   // Allowed as long as the browser can become the operating system default
   // browser.
-  return CanSetAsDefaultBrowser();
+  DefaultWebClientSetPermission permission = CanSetAsDefaultBrowser();
+
+  // Set as default asynchronous is only supported for default web browser.
+  return (permission == SET_DEFAULT_ASYNCHRONOUS) ? SET_DEFAULT_INTERACTIVE
+                                                  : permission;
 }
 
 static const struct ShellIntegration::AppModeInfo* gAppModeInfo = NULL;
@@ -124,6 +130,13 @@ bool ShellIntegration::SetAsDefaultBrowserInteractive() {
   return false;
 }
 
+#if !defined(OS_WIN)
+// static
+bool ShellIntegration::IsSetAsDefaultAsynchronous() {
+  return false;
+}
+#endif  // !defined(OS_WIN)
+
 // static
 bool ShellIntegration::SetAsDefaultProtocolClientInteractive(
     const std::string& protocol) {
@@ -152,51 +165,66 @@ 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_) {
+    if (set_as_default_initialized_)
+      FinalizeSetAsDefault();

[grt (UTC plus 2), 2015/10/02 17:39:18]

      set_as_default_initialized_ = false;
so that the variable represents the true state at all times

[Patrick Monette, 2015/10/02 20:15:13]

Done.

+
+    if (ShouldReportAttemptResults())
+      ReportAttemptResult(AttemptResult::RETRY);
+  }
+
+  set_as_default_in_progress_ = true;
   bool interactive_permitted = false;
   if (observer_) {
     observer_->SetDefaultWebClientUIState(STATE_PROCESSING);
     interactive_permitted = observer_->IsInteractiveSetDefaultPermitted();
+
+    // The initialization is only useful when there is an observer.
+    set_as_default_initialized_ = InitializeSetAsDefault();
   }
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DefaultWebClientWorker::ExecuteSetAsDefault, this,
-                 interactive_permitted));
+
+  // Remember the start time.
+  start_time_ = base::TimeTicks::Now();
+
+  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;
+  observer_ = nullptr;
+
+  if (set_as_default_initialized_) {
+    FinalizeSetAsDefault();
+    set_as_default_initialized_ = 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));
+ShellIntegration::DefaultWebClientWorker::~DefaultWebClientWorker() {
+  if (set_as_default_in_progress_ && ShouldReportAttemptResults())

[grt (UTC plus 2), 2015/10/02 17:39:18]

since this represents the observer walking away from the worker, i think it
makes sense to move this into ObserverDestroyed(). wdyt?

[Patrick Monette, 2015/10/02 20:15:13]

Done.

+    ReportAttemptResult(AttemptResult::ABANDONED);
 }
 
-void ShellIntegration::DefaultWebClientWorker::CompleteCheckIsDefault(
+void ShellIntegration::DefaultWebClientWorker::OnCheckIsDefaultComplete(
     DefaultWebClientState state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   UpdateUI(state);
@@ -204,30 +232,67 @@ void ShellIntegration::DefaultWebClientWorker::CompleteCheckIsDefault(
   // if we own it.
   if (observer_ && observer_->IsOwnedByWorker()) {
     delete observer_;
-    observer_ = NULL;
+    observer_ = nullptr;
   }
 }
 
-void ShellIntegration::DefaultWebClientWorker::ExecuteSetAsDefault(
-    bool interactive_permitted) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+void ShellIntegration::DefaultWebClientWorker::OnSetAsDefaultAttemptComplete(
+    AttemptResult result) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // Hold on to a reference because if this was called via the default browser
+  // callback in StartupBrowserCreator, clearing the callback in
+  // FinalizeSetAsDefault would otherwise remove the last reference and delete
+  // us in the middle of this function.
+  scoped_refptr<DefaultWebClientWorker> scoped_ref(this);
+
+  if (set_as_default_in_progress_) {
+    set_as_default_in_progress_ = false;
+
+    if (set_as_default_initialized_) {
+      FinalizeSetAsDefault();
+      set_as_default_initialized_ = false;
+    }
+    if (observer_) {
+      bool succeeded = result == AttemptResult::SUCCESS ||
+                       result == AttemptResult::ALREADY_DEFAULT;
+      observer_->OnSetAsDefaultConcluded(succeeded);
+    }
 
-  bool result = SetAsDefault(interactive_permitted);
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&DefaultWebClientWorker::CompleteSetAsDefault, this, result));
+    if (ShouldReportAttemptResults())
+      ReportAttemptResult(result);
+
+    // Start the default browser check which will notify the observer as to
+    // whether Chrome is really the default browser. This is needed because
+    // detecting that the process was successful is not 100% sure.
+    // For example, on Windows 10+, the user might have unchecked the "Always
+    // use this app" checkbox which can't be detected.
+    StartCheckIsDefault();
+  }
 }
 
-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();
+void ShellIntegration::DefaultWebClientWorker::ReportAttemptResult(
+    AttemptResult result) {
+  DCHECK(ShouldReportAttemptResults());

[grt (UTC plus 2), 2015/10/02 17:39:18]

how about:
  if (!ShouldReportAttemptResults())
    return;
here instead of checking at each callsite? i believe modern compilers should
still be able to optimize it away on non-Win platforms since the function will
become "if (true) return;"

[Patrick Monette, 2015/10/02 20:15:14]

Done.

+
+  UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.AsyncSetAsDefault.Result", result,
+                            AttemptResult::NUM_ATTEMPT_RESULT_TYPES);
+  UMA_HISTOGRAM_MEDIUM_TIMES("DefaultBrowser.AsyncSetAsDefault.Duration",
+                             base::TimeTicks::Now() - start_time_);

[grt (UTC plus 2), 2015/10/02 17:39:18]

the times we care about are:
- time to success (interaction was good)
- time to abandonment (interaction failed and user turned away)
- retry (interaction failed and user tried again)
please make these different histograms since we'll want to compare them with one
another.

[Patrick Monette, 2015/10/02 20:15:13]

Done.

+}
+
+bool ShellIntegration::DefaultWebClientWorker::InitializeSetAsDefault() {
+  return true;
 }
 
+void ShellIntegration::DefaultWebClientWorker::FinalizeSetAsDefault() {}
+
+#if !defined(OS_WIN)
+// static
+bool ShellIntegration::DefaultWebClientWorker::ShouldReportAttemptResults() {
+  return false;
+}
+#endif  // !defined(OS_WIN)
+
 void ShellIntegration::DefaultWebClientWorker::UpdateUI(
     DefaultWebClientState state) {
   if (observer_) {
@@ -256,30 +321,59 @@ ShellIntegration::DefaultBrowserWorker::DefaultBrowserWorker(
     : DefaultWebClientWorker(observer) {
 }
 
+ShellIntegration::DefaultBrowserWorker::~DefaultBrowserWorker() {}
+
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultBrowserWorker, private:
 
-ShellIntegration::DefaultWebClientState
-ShellIntegration::DefaultBrowserWorker::CheckIsDefault() {
-  return ShellIntegration::GetDefaultBrowser();
+void ShellIntegration::DefaultBrowserWorker::CheckIsDefault() {
+  DefaultWebClientState state = GetDefaultBrowser();
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultBrowserWorker::OnCheckIsDefaultComplete, this, state));
 }
 
-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();
+  AttemptResult result = AttemptResult::FAILURE;
+  switch (CanSetAsDefaultBrowser()) {
+    case SET_DEFAULT_NOT_ALLOWED:
+      NOTREACHED();
+      break;
+    case SET_DEFAULT_UNATTENDED:
+      if (SetAsDefaultBrowser())
+        result = AttemptResult::SUCCESS;
       break;
-    case ShellIntegration::SET_DEFAULT_INTERACTIVE:
-      if (interactive_permitted)
-        result = ShellIntegration::SetAsDefaultBrowserInteractive();
+    case SET_DEFAULT_INTERACTIVE:
+      if (interactive_permitted && SetAsDefaultBrowserInteractive())
+        result = AttemptResult::SUCCESS;
       break;
-    default:
+    case SET_DEFAULT_ASYNCHRONOUS:
+#if defined(OS_WIN)
+      if (!interactive_permitted)
+        break;
+      if (GetDefaultBrowser() == IS_DEFAULT) {
+        // Don't start the asynchronous operation since it could result in
+        // losing the default browser status.
+        result = AttemptResult::ALREADY_DEFAULT;
+        break;
+      }
+      // This function will cause OnSetAsDefaultAttemptComplete() to be called
+      // asynchronously via a filter established in InitializeSetAsDefault().
+      if (!SetAsDefaultBrowserAsynchronous()) {
+        result = AttemptResult::LAUNCH_FAILURE;
+        break;
+      }
+      return;
+#else
       NOTREACHED();
+      break;
+#endif
   }
-
-  return result;
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultBrowserWorker::OnSetAsDefaultAttemptComplete, this,
+                 result));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -295,28 +389,39 @@ ShellIntegration::DefaultProtocolClientWorker::DefaultProtocolClientWorker(
 ///////////////////////////////////////////////////////////////////////////////
 // DefaultProtocolClientWorker, private:
 
-ShellIntegration::DefaultWebClientState
-ShellIntegration::DefaultProtocolClientWorker::CheckIsDefault() {
-  return ShellIntegration::IsDefaultProtocolClient(protocol_);
+ShellIntegration::DefaultProtocolClientWorker::~DefaultProtocolClientWorker() {}
+
+void ShellIntegration::DefaultProtocolClientWorker::CheckIsDefault() {
+  DefaultWebClientState state = IsDefaultProtocolClient(protocol_);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultProtocolClientWorker::OnCheckIsDefaultComplete, 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;
+  AttemptResult result = AttemptResult::FAILURE;
+  switch (CanSetAsDefaultProtocolClient()) {
+    case SET_DEFAULT_NOT_ALLOWED:
+      // Not allowed, do nothing.
       break;
-    case ShellIntegration::SET_DEFAULT_UNATTENDED:
-      result = ShellIntegration::SetAsDefaultProtocolClient(protocol_);
+    case SET_DEFAULT_UNATTENDED:
+      if (SetAsDefaultProtocolClient(protocol_))
+        result = AttemptResult::SUCCESS;
       break;
-    case ShellIntegration::SET_DEFAULT_INTERACTIVE:
-      if (interactive_permitted) {
-        result = ShellIntegration::SetAsDefaultProtocolClientInteractive(
-            protocol_);
+    case SET_DEFAULT_INTERACTIVE:
+      if (interactive_permitted &&
+          SetAsDefaultProtocolClientInteractive(protocol_)) {
+        result = AttemptResult::SUCCESS;
       }
       break;
+    case SET_DEFAULT_ASYNCHRONOUS:
+      NOTREACHED();
+      break;
   }
-
-  return result;
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DefaultProtocolClientWorker::OnSetAsDefaultAttemptComplete,
+                 this, result));
 }