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

chrome/browser/ui/startup/startup_browser_creator.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/ui/startup/startup_browser_creator.h"
 
 #include <algorithm>   // For max().
 #include <set>
 
 #include "apps/app_load_service.h"
@@ skipping 88 matching lines @@
 #include "chrome/browser/printing/cloud_print/cloud_print_proxy_service.h"
 #include "chrome/browser/printing/cloud_print/cloud_print_proxy_service_factory.h"
 #include "chrome/browser/printing/print_dialog_cloud.h"
 #endif
 
 using content::BrowserThread;
 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:
   ProfileLaunchObserver()
       : profile_to_activate_(NULL),
         activated_profile_(false) {
     registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                    content::NotificationService::AllSources());
     registrar_.Add(this, chrome::NOTIFICATION_BROWSER_WINDOW_READY,
                    content::NotificationService::AllSources());
@@ skipping 320 matching lines @@
 // static
 void StartupBrowserCreator::RegisterLocalStatePrefs(
     PrefRegistrySimple* registry) {
 #if defined(OS_WIN)
   registry->RegisterStringPref(prefs::kLastWelcomedOSVersion, std::string());
   registry->RegisterBooleanPref(prefs::kWelcomePageOnOSUpgradeEnabled, true);
 #endif
 }
 
 // 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,
     Profile* profile) {
   std::vector<GURL> urls;
 
   const base::CommandLine::StringVector& params = command_line.GetArgs();
   for (size_t i = 0; i < params.size(); ++i) {
     base::FilePath param = base::FilePath(params[i]);
     // Handle Vista way of searching - "? <search-term>"
@@ skipping 45 matching lines @@
 }
 
 // static
 bool StartupBrowserCreator::ProcessCmdLineImpl(
     const base::CommandLine& command_line,
     const base::FilePath& cur_dir,
     bool process_startup,
     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);
   if (process_startup) {
     if (command_line.HasSwitch(switches::kDisablePromptOnRepost))
       content::NavigationController::DisablePromptOnRepost();
   }
 
   bool silent_launch = false;
 
@@ skipping 119 matching lines @@
     // Return early here since we don't want to open a browser window.
     // The exception is when there are no browser windows, since we don't want
     // chrome to shut down.
     // TODO(jackhou): Do this properly once keep-alive is handled by the
     // background page of apps. Tracked at http://crbug.com/175381
     if (chrome::GetTotalBrowserCountForProfile(last_used_profile) != 0)
       return true;
   }
 
 #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 :
       chrome::startup::IS_NOT_PROCESS_STARTUP;
   chrome::startup::IsFirstRun is_first_run = first_run::IsChromeFirstRun() ?
       chrome::startup::IS_FIRST_RUN : chrome::startup::IS_NOT_FIRST_RUN;
   // |last_opened_profiles| will be empty in the following circumstances:
   // - This is the first launch. |last_used_profile| is the initial profile.
   // - The user exited the browser by closing all windows for all
   // profiles. |last_used_profile| is the profile which owned the last open
@@ skipping 173 matching lines @@
   // If we are showing the app list then chrome isn't shown so load the app
   // list's profile rather than chrome's.
   if (command_line.HasSwitch(switches::kShowAppList)) {
     return AppListService::Get(chrome::HOST_DESKTOP_TYPE_NATIVE)->
         GetProfilePath(user_data_dir);
   }
 
   return g_browser_process->profile_manager()->GetLastUsedProfileDir(
       user_data_dir);
 }