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

chrome/browser/shell_integration_win.cc

Index: chrome/browser/shell_integration_win.cc
diff --git a/chrome/browser/shell_integration_win.cc b/chrome/browser/shell_integration_win.cc
index 07f7b4b9f8e465faaad7240654e82ebedbb91644..ad9f99e340f14e6f171708fd626b463999b2a3ae 100644
--- a/chrome/browser/shell_integration_win.cc
+++ b/chrome/browser/shell_integration_win.cc
@@ -14,7 +14,10 @@
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
+#include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -25,6 +28,7 @@
 #include "base/win/shortcut.h"
 #include "base/win/windows_version.h"
 #include "chrome/browser/policy/policy_path_parser.h"
+#include "chrome/browser/ui/startup/startup_browser_creator.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths_internal.h"
@@ -38,6 +42,7 @@
 #include "chrome/installer/util/util_constants.h"
 #include "chrome/installer/util/work_item.h"
 #include "chrome/installer/util/work_item_list.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
@@ -46,6 +51,10 @@ namespace {
 
 const wchar_t kAppListAppNameSuffix[] = L"AppList";
 
+const char kAsyncSetAsDefaultExperimentName[] = "AsyncSetAsDefault";
+const char kEnableAsyncSetAsDefault[] = "enable-async-set-as-default";
+const char kDisableAsyncSetAsDefault[] = "disable-async-set-as-default";
+
 // Helper function for ShellIntegration::GetAppId to generates profile id
 // from profile path. "profile_id" is composed of sanitized basenames of
 // user data dir and profile dir joined by a ".".
@@ -240,8 +249,59 @@ ShellIntegration::DefaultWebClientState
   }
 }
 
+// Resets the default browser choice for the current user.
+void ResetDefaultBrowser() {
+  static const wchar_t* const kUrlAssociationKeyFormats[] = {
+      L"SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\"
+      L"%ls\\UserChoice",

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Can we replace the "ls" params in these with "s" and use char* for the
protocols?

[Patrick Monette, 2015/09/28 23:46:37]

No, the types must match in StringPrintf

[Peter Kasting, 2015/09/28 23:57:49]

I'm confused.  I am suggesting that the types match -- that these params specify
narrow strings and the protocols provided for those params be narrow strings.

Are you saying the wchar_t*-format string StringPrintf() cannot accept %s
arguments at all?

[Patrick Monette, 2015/09/29 14:44:18]

Yes. Apparently it is because the %s specifier is non-portable between
platforms. Even if this is Windows-specific code, a DCHECK fails at runtime on
IsWprintfFormatPortable() even if it compiles with no warnings.

More info:
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

+      L"SOFTWARE\\Microsoft\\Windows\\Roaming\\OpenWith\\UrlAssociations\\"
+      L"%ls\\UserChoice"};
+  static const wchar_t* const kProtocols[] = {L"http", L"https"};
+
+  for (const wchar_t* format : kUrlAssociationKeyFormats) {
+    for (const wchar_t* protocol : kProtocols) {
+      base::win::RegKey registry_key(
+          HKEY_CURRENT_USER, base::StringPrintf(format, protocol).c_str(),
+          KEY_SET_VALUE);
+      registry_key.DeleteValue(L"Hash");
+    }
+  }
+}
+
+// Returns true if the AsyncSetAsDefault field trial is activated.
+bool IsAsyncSetAsDefaultEnabled() {
+  using base::CommandLine;
+
+  const std::string group_name =

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Init this just above its first use.

At that point I might just inline this into the statement that uses it.

[Patrick Monette, 2015/09/28 23:46:38]

There is a comment about that in the finch101 guide (go/finch101). I'll add it
here.

+      base::FieldTrialList::FindFullName("AsyncSetAsDefault");
+  if (CommandLine::ForCurrentProcess()->HasSwitch(kDisableAsyncSetAsDefault))
+    return false;
+  if (CommandLine::ForCurrentProcess()->HasSwitch(kEnableAsyncSetAsDefault))
+    return true;
+
+  return base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE);
+}
+
 }  // namespace
 
+// static
+bool ShellIntegration::IsSetAsDefaultAsynchronous() {
+  return base::win::GetVersion() >= base::win::VERSION_WIN10 &&
+         IsAsyncSetAsDefaultEnabled();
+}
+
+void ShellIntegration::SetAsDefaultBrowserAsynchronous() {
+  DCHECK(ShellIntegration::IsSetAsDefaultAsynchronous());
+
+  ResetDefaultBrowser();
+
+  base::CommandLine cmdline(base::FilePath(L"openwith.exe"));
+  cmdline.AppendArgNative(StartupBrowserCreator::GetDefaultBrowserUrl());
+  base::Process process(base::LaunchProcess(cmdline, base::LaunchOptions()));
+  UMA_HISTOGRAM_BOOLEAN("DefaultBrowser.AsyncSetAsDefault.Launched",
+                        process.IsValid());
+}
+
 ShellIntegration::DefaultWebClientSetPermission
     ShellIntegration::CanSetAsDefaultBrowser() {
   BrowserDistribution* distribution = BrowserDistribution::GetDistribution();
@@ -251,8 +311,10 @@ ShellIntegration::DefaultWebClientSetPermission
 
   if (ShellUtil::CanMakeChromeDefaultUnattended())
     return SET_DEFAULT_UNATTENDED;
-  else
-    return SET_DEFAULT_INTERACTIVE;
+  if (IsSetAsDefaultAsynchronous())

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Blank line above this, or else remove all the blank lines in this function
(no obvious rule on how these blanks are being used).

Shorter:

  return IsSetAsDefaultAsynchronous() ?
      SET_DEFAULT_ASYNCHRONOUS : SET_DEFAULT_INTERACTIVE;

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    return SET_DEFAULT_ASYNCHRONOUS;
+
+  return SET_DEFAULT_INTERACTIVE;
 }
 
 bool ShellIntegration::SetAsDefaultBrowser() {
@@ -588,3 +650,66 @@ base::FilePath ShellIntegration::GetStartMenuShortcut(
 
   return base::FilePath();
 }
+
+void ShellIntegration::DefaultBrowserWorker::InitializeSetAsDefault() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (IsSetAsDefaultAsynchronous()) {

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Reverse this + early return to avoid indenting the entire function body

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    // On windows 10+, there is no official way to prompt the user to set a

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Capitalize Windows

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    // default browser. This is the workaround:
+    // 1. Unregister the default browser.
+    // 2. Open "How to make Chrome my default browser" link with openwith.exe.
+    // 3. Windows prompt the user with "How would you link to open this?".

[Peter Kasting, 2015/09/25 20:52:27]

Nit: prompt -> will prompt

[Patrick Monette, 2015/09/28 23:46:38]

Done.

+    // 4. If Chrome is selected, the http url is intercepted and
+    //    CompleteSetAsDefault is called with succeeded equals to true.

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Avoid passive voice, plus grammar issues.  How about "If Chrome is
selected, we intercept the attempt to open the URL and instead call
CompleteSetAsDefault(), passing true to indicate success."

[Patrick Monette, 2015/09/28 23:46:38]

Done.

+    // 5. If Chrome is not selected, the url is opened in the selected browser.
+    //    After a certain amount of time, we notify the observer that the
+    //    process failed.
+
+    // Remember the start time.
+    start_time_ = base::TimeTicks::Now();
+
+    if (!StartupBrowserCreator::SetDefaultBrowserCallback(base::Bind(
+            &DefaultBrowserWorker::CompleteSetAsDefault, this, true))) {
+      // Another worker is currently processing. Complete here and leave the
+      // other worker alone.
+      CompleteSetAsDefault(false);

[grt (UTC plus 2), 2015/09/28 14:31:04]

i don't think this is right. when InitializeSetAsDefault() returns,
StartSetAsDefault() will post a task to the FILE thread that calls SetAsDefault.

[Patrick Monette, 2015/09/28 23:46:38]

I could set add if (set_as_default_in_progress_) before posting a task for
SetAsDefault.
One potentially bad side-effect of that is that the following flow won't work :
1. chrome://settings
2. Click Make Chrome the default browser.
3. (The timer starts, the callback is set, the popup is opened)
4. Dismiss the popup (the timer still runs and the callback is set)
5. Click Set as default using the butter bar at the top.

Or I leave it like that and the preceding flow will work. I just need to add a
comment to explain that it is intended.

What do you think?

+      return;
+    }
+
+    // Start the timer.
+    if (!async_timer_)
+      async_timer_.reset(new base::OneShotTimer());

[Peter Kasting, 2015/09/25 20:52:27]

Nit: Because this doesn't actually start the timer, and the comment above is
meant to refer to all the code from here on, remove the two blank lines below
this.

[Patrick Monette, 2015/09/28 23:46:38]

Done.

+
+    base::TimeDelta timer_duration = base::TimeDelta::FromMinutes(2);

[Peter Kasting, 2015/09/25 20:52:27]

Nit: If you move this down lower you can init as:

  base::TimeDelta timer_duration = seconds ?
      base::TimeDelta::FromSeconds(seconds) : base::TimeDelta::FromMinutes(2);

Another way to do this:

  if (!seconds)
    seconds = 120;
  async_timer_->Start(
      FROM_HERE, base::TimeDelta::FromSeconds(seconds),
      base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this, false));

[Patrick Monette, 2015/09/28 23:46:37]

Done.

+    std::string value = variations::GetVariationParamValue(
+        kAsyncSetAsDefaultExperimentName, "TimerDuration");
+    int seconds = 0;
+    if (!value.empty())
+      base::StringToInt(value, &seconds);
+    if (seconds)
+      timer_duration = base::TimeDelta::FromSeconds(seconds);
+
+    async_timer_->Start(
+        FROM_HERE, timer_duration,
+        base::Bind(&DefaultBrowserWorker::CompleteSetAsDefault, this, false));
+  }
+}
+
+void ShellIntegration::DefaultBrowserWorker::FinalizeSetAsDefault(
+    bool succeeded) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  if (async_timer_) {
+    // Destroy timer.

[Peter Kasting, 2015/09/25 20:52:27]

Nit: This comment and the next one both simply restate the code here; remove
them.

[Patrick Monette, 2015/09/28 23:46:38]

Done.

+    async_timer_.reset();
+
+    StartupBrowserCreator::ClearDefaultBrowserCallback();
+
+    // Record the duration from starting to set as default browser to now.
+    UMA_HISTOGRAM_BOOLEAN("DefaultBrowser.AsyncSetAsDefault.Succeeded",
+                          succeeded);
+    UMA_HISTOGRAM_TIMES("DefaultBrowser.AsyncSetAsDefault.Duration",
+                        base::TimeTicks::Now() - start_time_);
+  }
+}