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..05bd5db6fb1b3d329294355d1589802830bb1349 100644
--- a/chrome/browser/shell_integration_win.cc
+++ b/chrome/browser/shell_integration_win.cc
@@ -11,16 +11,20 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <vector>
+
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.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 +219,96 @@ 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 the protocol
+// association with an app are also monitored to signal the end of the
+// interaction early when it is clear that the user made a choice (e.g. http
+// and https for default browser).
+//
+// 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.
+// This class also manages its own lifetime.
+class OpenSystemSettingsHelper {
+ public:
+  OpenSystemSettingsHelper(const std::vector<const wchar_t*>& protocols,
+                           const base::Closure& on_finished_callback)
+      : on_finished_callback_(on_finished_callback),
+        registry_watcher_count_(protocols.size()),

[grt (UTC plus 2), 2016/03/31 13:23:37]

should this count be the # of protocols provided, or the # of watchers
successfully created? in other words, if the helper is asked to monitor two
protocols but can only monitor one, should it complete the interaction after the
one is notified, or wait until the timeout?

[Patrick Monette, 2016/03/31 18:26:03]

Okay I changed it to the # of watchers successfully created. I don't think the
differences between the 2 are important but a watcher is essentially useless if
another one failed and we're waiting on the 2.

+        weak_ptr_factory_(this) {
+    static const wchar_t kUrlAssociationFormat[] =
+        L"SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\"
+        L"%ls\\UserChoice";
+
+    registry_key_watchers_.reserve(registry_watcher_count_);
+    for (const wchar_t* protocol : protocols) {
+      registry_key_watchers_.push_back(CreateRegistryKeyWatcher(
+          base::StringPrintf(kUrlAssociationFormat, protocol).c_str()));
+    }
+
+    timer_.Start(FROM_HERE, base::TimeDelta::FromMinutes(2),
+                 base::Bind(&OpenSystemSettingsHelper::
+                                ConcludeOpenSystemSettingsToDefaultBrowser,
+                            weak_ptr_factory_.GetWeakPtr()));
+  }
+
+ private:
+  void OnRegistryValueChanged() {

[grt (UTC plus 2), 2016/03/31 13:23:37]

nit: OnRegistryKeyChanged since it monitors a key not a value. also worth noting
somewhere in a comment that this helper only waits for the first modification of
any type to a key (value changes, attribute changes, ACL changes, etc). this is
likely "good enough" for now, but i think it's worth noting.

[Patrick Monette, 2016/03/31 18:26:03]

Done.

+    DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+
+    // Make sure all the registry watchers fired.

[grt (UTC plus 2), 2016/03/31 13:23:37]

nit: "have fired"

[Patrick Monette, 2016/03/31 18:26:03]

Done.

+    registry_watcher_count_--;
+
+    if (registry_watcher_count_ == 0)

[grt (UTC plus 2), 2016/03/31 13:23:37]

nit: if (--registry_watcher_count_ == 0)

[Patrick Monette, 2016/03/31 18:26:03]

Done.

+      ConcludeOpenSystemSettingsToDefaultBrowser();
+  }
+
+  void ConcludeOpenSystemSettingsToDefaultBrowser() {

[grt (UTC plus 2), 2016/03/31 13:23:37]

nit: add a doc comment

[grt (UTC plus 2), 2016/03/31 13:23:37]

since this will be re-used for other protocols, consider generalizing this
method name ("DefaultBrowser"). maybe just "ConcludeInteraction"?

[Patrick Monette, 2016/03/31 18:26:03]

Done.

[Patrick Monette, 2016/03/31 18:26:04]

Done.

+    DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+
+    on_finished_callback_.Run();
+    delete this;
+  }
+
+  scoped_ptr<base::win::RegKey> CreateRegistryKeyWatcher(

[grt (UTC plus 2), 2016/03/31 13:23:37]

nit: add a doc comment

[Patrick Monette, 2016/03/31 18:26:03]

Done.

+      const wchar_t* registry_value) {

[grt (UTC plus 2), 2016/03/31 13:23:37]

nit: key_path since it's not naming a value in the registry

[Patrick Monette, 2016/03/31 18:26:03]

Done.

+    scoped_ptr<base::win::RegKey> reg_key(
+        new base::win::RegKey(HKEY_CURRENT_USER, registry_value, KEY_READ));
+
+    if (reg_key->Valid()) {

[grt (UTC plus 2), 2016/03/31 13:23:37]

only return the key if it's really being watched?

if (reg_key->Valid() && StartWatching(...))
  return reg_key;
return scoped_ptr<base::win::RegKey>();

otherwise the helper will always wait for the timeout. or is that what you
really want?

[Patrick Monette, 2016/03/31 18:26:03]

I changed it to AddRegistryKeyWatcher() so the vector only keep tracks of
initialized watchers.

+      reg_key->StartWatching(
+          base::Bind(&OpenSystemSettingsHelper::OnRegistryValueChanged,
+                     weak_ptr_factory_.GetWeakPtr()));
+    }
+    return reg_key;
+  }
+
+  // The function to call when the interaction with the system settings is
+  // finished.
+  base::Closure on_finished_callback_;
+
+  // The number of time the registry key watchers must fire.
+  int registry_watcher_count_;
+
+  // There can be multiple registry key watchers as some settings modify
+  // multiple protocol associations. e.g. Changing the default browser modify

[grt (UTC plus 2), 2016/03/31 13:23:37]

nit: "modifies"

[Patrick Monette, 2016/03/31 18:26:03]

Done.

+  // the http and https associations.
+  std::vector<scoped_ptr<base::win::RegKey>> registry_key_watchers_;
+
+  base::OneShotTimer timer_;
+
+  // Weak ptrs are used to bind this class to the callbacks of the timer and the
+  // registry watcher. This makes it possible to self-delete after one of the
+  // callbacks is executed to cancel the remaining ones.
+  base::WeakPtrFactory<OpenSystemSettingsHelper> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(OpenSystemSettingsHelper);
+};
+
 }  // namespace
 
 bool SetAsDefaultBrowser() {
@@ -253,6 +347,23 @@ bool SetAsDefaultBrowserInteractive() {
   return true;
 }
 
+void SetAsDefaultBrowserUsingSystemSettings(
+    const base::Closure& on_finished_callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+
+  // The helper manages its own lifetime.
+  new OpenSystemSettingsHelper({L"http", L"https"}, on_finished_callback);

[grt (UTC plus 2), 2016/03/31 13:23:37]

note: uniform initialization is banned
(http://chromium-cpp.appspot.com/#core-blacklist)

[grt (UTC plus 2), 2016/03/31 13:23:37]

rather than the naked new, i suggest a static function on the helper class to
kick things off:

  static const wchar_t* const kProtocols[] = {L"http", L"https", nullptr};
  OpenSystemSettingsHelper::Begin(kProtocols, on_finished_callback);

the ctor can then be a private method

[Patrick Monette, 2016/03/31 18:26:03]

Done.

[Patrick Monette, 2016/03/31 18:26:03]

Done.

+
+  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 +413,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;
 }