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

chrome/browser/ui/startup/startup_browser_creator.cc

Index: chrome/browser/ui/startup/startup_browser_creator.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator.cc b/chrome/browser/ui/startup/startup_browser_creator.cc
index f7fac34905f789c96ce28d0107dca525986da7ce..6c29b2e4f4777ac533ed6ca4ddf35f2688949473 100644
--- a/chrome/browser/ui/startup/startup_browser_creator.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator.cc
@@ -106,6 +106,12 @@ using content::ChildProcessSecurityPolicy;
 
 namespace {
 
+const wchar_t kSetDefaultBrowserHelpUrl[] =
+    L"https://support.google.com/chrome?p=default_browser";

[grt (UTC plus 2), 2015/09/24 18:47:06]

PK: it seems like this should be brand-specific. do you think this should come
from google_chrome_strings.grd with some other value in chromium_strings.grd, or
just put this in a #if defined(GOOGLE_CHROME_BUILD) in some way?

[Peter Kasting, 2015/09/24 20:34:10]

Do we have any support pages for Chromium (not Chrome)?  What do we do for other
help/support links in the codebase?

+
+// Not thread-safe. Always use or modify this callback on the UI thread.
+base::CancelableClosure async_set_as_default_filter_callback;

[Peter Kasting, 2015/09/23 23:01:39]

The Google style guide bans the use of global or static objects of non-POD type.

You could use a pointer to a closure and set it to null when it's canceled. 
That avoids the complexity of CancelableClosure anyway, which you don't actually
need; just make sure not to leak memory.

You could also just store a DefaultBrowserWorker* and call
CompleteSetAsDefault() on it directly, which I find noticeably simpler and
clearer, at the cost of a conceptual dependency that to me seems acceptable to
introduce, since this code already is understanding that this has to do with
handling being set as the default browser.

Either one of these would probably simplify your implementation enough to inline
the accessors in the header.

[grt (UTC plus 2), 2015/09/24 18:24:12]

i think CancelableClosure is overkill here since you never need to cancel a
callback that is in-flight (posted to a message loop). i think it would suffice
to have:

base::Closure* g_async_set_as_default_callback = nullptr;

and:

SetAsyncSetAsDefaultFilter(const base::Closure& callback) {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  if (callback.is_null()) {
    delete g_async_set_as_default_callback;
    g_async_set_as_default_callback = nullptr;
    return;
  }
  if (!g_async_set_as_default_callback)
    g_async_set_as_default_callback = new base::Closure(callback);
  else
    *g_async_set_as_default_callback = callback;
}

and get rid of ClearAsyncSetAsDefaultFilter (callers can pass an empty callback
to clear).

one thing to keep in mind is that it's pretty easy to get into a situation where
a second worker is started while a first worker is still waiting for the
timeout. have you thought about how that will be handled? 

[Peter Kasting, 2015/09/24 20:34:10]

One downside to this is it's prone to leaks.  It's not clear who's supposed to
delete this if no one calls the API with a null pointer (e.g. we shut down
first).

One reason I liked my DefaultBrowserWorker* suggestion is that not only do I
think it's simpler and clearer, there aren't lifetime problems, because this
would be a non-owning pointer, so it can't leak.

[grt (UTC plus 2), 2015/09/24 20:39:24]

On the flipside, it means that there's a risk that the Worker goes away without
clearing the pointer here, leaving a dangling pointer. Then you'll get a crash
on a subsequent navigation to the magic URL.

I'm less concerned about leaking since there's no risk of leaking more than one
instance, and we have a policy of leaking global-ish objects at shutdown.

[Patrick Monette, 2015/09/25 20:06:25]

Done.

+
 // Keeps track on which profiles have been launched.
 class ProfileLaunchObserver : public content::NotificationObserver {
  public:
@@ -446,6 +452,23 @@ void StartupBrowserCreator::RegisterLocalStatePrefs(
 }
 
 // static
+void StartupBrowserCreator::SetAsyncSetAsDefaultFilter(base::Closure callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  async_set_as_default_filter_callback.Reset(callback);
+}
+
+// static
+void StartupBrowserCreator::ClearAsyncSetAsDefaultFilter() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  async_set_as_default_filter_callback.Cancel();
+}
+
+// static
+const wchar_t* StartupBrowserCreator::GetAsyncSetAsDefaultUrl() {
+  return kSetDefaultBrowserHelpUrl;
+}
+
+// static
 std::vector<GURL> StartupBrowserCreator::GetURLsFromCommandLine(
     const base::CommandLine& command_line,
     const base::FilePath& cur_dir,
@@ -511,6 +534,7 @@ bool StartupBrowserCreator::ProcessCmdLineImpl(
     Profile* last_used_profile,
     const Profiles& last_opened_profiles,
     StartupBrowserCreator* browser_creator) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   TRACE_EVENT0("startup", "StartupBrowserCreator::ProcessCmdLineImpl");
 
   DCHECK(last_used_profile);
@@ -650,12 +674,25 @@ bool StartupBrowserCreator::ProcessCmdLineImpl(
   }
 
 #if defined(OS_WIN)
+  // Intercept a specific url when setting the default browser asynchronously.
+  // This only happens on Windows 10+.
+  if (!async_set_as_default_filter_callback.IsCancelled()) {
+    for (const auto& arg : command_line.GetArgs()) {
+      GURL url(arg);

[Peter Kasting, 2015/09/23 23:01:39]

Nit: I wonder if it wouldn't be simpler to just string-compare, which would
avoid the overhead (and the code) of creating a canonical URL out of these.

[Patrick Monette, 2015/09/25 20:06:25]

Done.

+      if (url.is_valid() &&
+          url == GURL(L"https://support.google.com/chrome?p=default_browser")) {

[Peter Kasting, 2015/09/23 23:01:39]

Why are you not using the constant here?

[Patrick Monette, 2015/09/25 20:06:25]

Using it now.

+        async_set_as_default_filter_callback.callback().Run();
+        return true;
+      }
+    }
+  }
+
   // Log whether this process was a result of an action in the Windows Jumplist.
   if (command_line.HasSwitch(switches::kWinJumplistAction)) {
     jumplist::LogJumplistActionFromSwitchValue(
         command_line.GetSwitchValueASCII(switches::kWinJumplistAction));
   }
-#endif
+#endif  // defined(OS_WIN)
 
   chrome::startup::IsProcessStartup is_process_startup = process_startup ?
       chrome::startup::IS_PROCESS_STARTUP :