DefaultBrowserWorker now fully supports opening the settings for Win10

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 fc5cba989436839de7119bfec218bc053154bccc..b71c6cbdf52358005661ccd7b959127416bc8d14 100644
--- a/chrome/browser/shell_integration_win.cc
+++ b/chrome/browser/shell_integration_win.cc
@@ -16,11 +16,13 @@
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/timer/timer.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/scoped_propvariant.h"
@@ -215,6 +217,104 @@ DefaultWebClientState GetDefaultWebClientStateFromShellUtilDefaultState(
   }
 }
 
+bool IsSetAsDefaultUsingSystemSettings() {
+  return base::win::GetVersion() >= base::win::VERSION_WIN10;
+}
+
+// There is no way to make sure the user is done with the system settings, but a
+// signal that the interaction is finished is needed for UMA. A timer of 2
+// minutes is used as a substitute. The registry values for default browser is
+// also monitored (http and https association) to signal the end of the
+// interaction early when it is clear that the user made a choice.
+//
+// This helper class manages both the timer and the registry watchers and makes
+// sure the callback for the end of the settings interaction is only run once.
+class OpenSystemSettingsHelper
+    : public base::RefCounted<OpenSystemSettingsHelper> {
+ public:
+  OpenSystemSettingsHelper() {}
+
+  // Initializes the timer and the registry watcher.
+  void Initialize(const base::Closure& on_finished_callback) {

[grt (UTC plus 2), 2016/03/30 15:55:09]

is there a reason not to move this into the ctor and get rid of Initialize()?

[Patrick Monette, 2016/03/30 22:21:55]

No reason. Changed it!

+    const wchar_t url_association_format[] =

[grt (UTC plus 2), 2016/03/30 15:55:10]

nit: static const wchar_t kUrlAssociationFormat[]

[Patrick Monette, 2016/03/30 22:21:55]

Done.

+        L"SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\"
+        L"%ls\\UserChoice";
+
+    base::Callback<void(bool)> conclude_settings_interaction_callback =
+        base::Bind(&OpenSystemSettingsHelper::
+                       ConcludeOpenSystemSettingsToDefaultBrowser,
+                   this, on_finished_callback);

[grt (UTC plus 2), 2016/03/30 15:55:10]

what's the advantage of binding on_finished_callback into the callbacks passed
to the watcher and timer? could you put it in a member variable in the helper
instead?

[Patrick Monette, 2016/03/30 22:21:55]

Done.

+
+    http_registry_key_ = CreateRegistryKeyWatcher(
+        base::StringPrintf(url_association_format, L"http").c_str(),
+        conclude_settings_interaction_callback);
+    https_registry_key_ = CreateRegistryKeyWatcher(
+        base::StringPrintf(url_association_format, L"https").c_str(),
+        conclude_settings_interaction_callback);
+
+    timer_.Start(FROM_HERE, base::TimeDelta::FromMinutes(2),
+                 base::Bind(conclude_settings_interaction_callback, false));
+  }
+
+  // Either the timer or the registry watcher signaled the end of the
+  // interaction. Make sure the other signal is ignored.
+  void Clear() {
+    run_once_ = false;
+    timer_.Reset();
+    http_registry_key_.reset();
+    https_registry_key_.reset();
+  }
+
+ private:
+  void ConcludeOpenSystemSettingsToDefaultBrowser(
+      const base::Closure& on_finished_callback,
+      bool is_registry_watcher) {

[grt (UTC plus 2), 2016/03/30 15:55:09]

i think the code would be a little more readable if the "a reg key was modified"
callback and the "we timed out" callback were handled by two different
(well-named) functions.

[Patrick Monette, 2016/03/30 22:21:55]

Done for "a reg key was modified" and kept
ConcludeOpenSystemSettingsToDefaultBrowser().

+    DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+
+    // It is possible to get here if only one of the http/https url association
+    // was changed, which is not enough for GetDefaultBrowser() to return
+    // IS_DEFAULT.
+    if (is_registry_watcher) {
+      registry_watcher_count_++;
+      if (registry_watcher_count_ != 2)
+        return;
+    }
+
+    if (!run_once_)

[grt (UTC plus 2), 2016/03/30 15:55:10]

you can do away with |run_once_| if you keep on_finished_callback as a member
variable by clearing out on_finished_callback after it is first invoked:

    if (on_finished_callback_.is_null())
      return;
    on_finished_callback_.Run();
    Reset();

(i like Reset more than Clear since that's what happens under the covers.)

and in Reset():
    on_finished_callback_ = base::Closure();

better yet: does this class need to be refcounted? i think you get what you need
by binding via WeakPtrs and deleting the instance after invoking the callback:

      on_finished_callback_.Run();
      delete this;

then you don't need Clear/Reset at all since the dtor will take care of
everything for you.

[Patrick Monette, 2016/03/30 22:21:55]

Done. The naked new in SetAsDefaultBrowserUsingSystemSettings is weird? But I
think you know best.

+      return;
+    Clear();
+
+    on_finished_callback.Run();
+  }
+
+  scoped_ptr<base::win::RegKey> CreateRegistryKeyWatcher(

[grt (UTC plus 2), 2016/03/30 15:55:09]

wdyt of getting rid of the scoped_ptrs and changing this to something like:
  static void InstallRegistryKeyWatcher(
      const wchar_t* key_path,
      const base::Closure& change_callback,
      base::win::RegKey* key) {
    if (key->Open(...) == ERROR_SUCCESS &&
        !key->StartWatching(change_callback)) {
      key->Close();
    }
  }

[Patrick Monette, 2016/03/30 22:21:55]

Kept this as-is to use with the new vector of scoped ptrs.

+      const wchar_t* registry_value,
+      const base::Callback<void(bool)>&
+          conclude_settings_interaction_callback) {
+    scoped_ptr<base::win::RegKey> reg_key(
+        new base::win::RegKey(HKEY_CURRENT_USER, registry_value, KEY_READ));
+
+    if (reg_key->Valid()) {
+      reg_key->StartWatching(
+          base::Bind(conclude_settings_interaction_callback, true));
+    }
+    return reg_key;
+  }
+
+  base::OneShotTimer timer_;
+  scoped_ptr<base::win::RegKey> http_registry_key_;
+  scoped_ptr<base::win::RegKey> https_registry_key_;
+
+  // Used to make sure only one of the timer or the registry watcher's signal is
+  // taken in consideration.
+  bool run_once_ = true;
+
+  // The number of time a registry key watcher fired.
+  int registry_watcher_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(OpenSystemSettingsHelper);
+};
+
 }  // namespace
 
 bool SetAsDefaultBrowser() {
@@ -253,6 +353,23 @@ bool SetAsDefaultBrowserInteractive() {
   return true;
 }
 
+void SetAsDefaultBrowserUsingSystemSettings(
+    const base::Closure& on_finished_callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+
+  scoped_refptr<OpenSystemSettingsHelper> helper(new OpenSystemSettingsHelper);
+  helper->Initialize(on_finished_callback);
+
+  base::FilePath chrome_exe;
+  if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
+    NOTREACHED() << "Error getting app exe path";
+    return;
+  }
+
+  BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+  ShellUtil::ShowMakeChromeDefaultSystemUI(dist, chrome_exe);
+}
+
 bool SetAsDefaultProtocolClient(const std::string& protocol) {
   if (protocol.empty())
     return false;
@@ -302,6 +419,8 @@ DefaultWebClientSetPermission CanSetAsDefaultBrowser() {
     return SET_DEFAULT_NOT_ALLOWED;
   if (ShellUtil::CanMakeChromeDefaultUnattended())
     return SET_DEFAULT_UNATTENDED;
+  if (IsSetAsDefaultUsingSystemSettings())
+    return SET_DEFAULT_OPEN_SETTINGS;
   return SET_DEFAULT_INTERACTIVE;
 }