Do not register Chrome's DelegateExecute on Win10+.

chrome/installer/util/shell_util.cc

Index: chrome/installer/util/shell_util.cc
diff --git a/chrome/installer/util/shell_util.cc b/chrome/installer/util/shell_util.cc
index 46defa119135f89d76b0d7b2578fecf14bdeea31..26140bbef0676111297829e1d3db899961ef864b 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -77,29 +77,12 @@ enum RegistrationConfirmationLevel {
 
 const wchar_t kReinstallCommand[] = L"ReinstallCommand";
 
-// Returns true if Chrome Metro is supported on this OS (Win 8 8370 or greater).
-// TODO(gab): Change this to a simple check for Win 8 once old Win8 builds
-// become irrelevant.
+// Returns true if Chrome Metro is supported on this version of Windows
+// (supported as of Win8; deprecated as of Win10).
 bool IsChromeMetroSupported() {
-  OSVERSIONINFOEX min_version_info = {};
-  min_version_info.dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX);
-  min_version_info.dwMajorVersion = 6;
-  min_version_info.dwMinorVersion = 2;
-  min_version_info.dwBuildNumber = 8370;
-  min_version_info.wServicePackMajor = 0;
-  min_version_info.wServicePackMinor = 0;
-
-  DWORDLONG condition_mask = 0;
-  VER_SET_CONDITION(condition_mask, VER_MAJORVERSION, VER_GREATER_EQUAL);
-  VER_SET_CONDITION(condition_mask, VER_MINORVERSION, VER_GREATER_EQUAL);
-  VER_SET_CONDITION(condition_mask, VER_BUILDNUMBER, VER_GREATER_EQUAL);
-  VER_SET_CONDITION(condition_mask, VER_SERVICEPACKMAJOR, VER_GREATER_EQUAL);
-  VER_SET_CONDITION(condition_mask, VER_SERVICEPACKMINOR, VER_GREATER_EQUAL);
-
-  DWORD type_mask = VER_MAJORVERSION | VER_MINORVERSION | VER_BUILDNUMBER |
-      VER_SERVICEPACKMAJOR | VER_SERVICEPACKMINOR;
-
-  return VerifyVersionInfo(&min_version_info, type_mask, condition_mask) != 0;
+  const base::win::Version win_version = base::win::GetVersion();
+  return win_version >= base::win::VERSION_WIN8 &&
+         win_version < base::win::VERSION_WIN10;
 }
 
 // Returns the current (or installed) browser's ProgId (e.g.
@@ -192,6 +175,16 @@ class RegistryEntry {
     LOOK_IN_HKCU_THEN_HKLM = LOOK_IN_HKCU | LOOK_IN_HKLM,
   };
 
+  // Identifies the type of removal this RegistryEntry is flagged for if any.

[grt (UTC plus 2), 2015/08/24 15:08:20]

"...for, if any."

[gab, 2015/08/27 20:51:07]

Done.

+  enum class RemovalFlag {
+    // Default case, no flag.

[grt (UTC plus 2), 2015/08/24 15:08:19]

// Default case: install the key/value.

[gab, 2015/08/27 20:51:07]

Done.

+    NONE,
+    // Registry value under |key_path_|\|name_| is flagged for deletion.
+    VALUE,
+    // Registry key under |key_path_| is flag for deletion.
+    KEY,
+  };
+
   // Details about a Windows application, to be entered into the registry for
   // the purpose of file associations.
   struct ApplicationInfo {
@@ -251,6 +244,68 @@ class RegistryEntry {
     return GetBrowserClientKey(dist, suffix).append(L"\\Capabilities");
   }
 
+  // DelegateExecute ProgId. Needed for Chrome Metro in Windows 8. This is
+  // only needed for registring a web browser, not for general associations.
+  static ScopedVector<RegistryEntry> GetChromeDelegateExecuteEntries(
+      const base::FilePath& chrome_exe,
+      const ApplicationInfo& app_info) {
+    ScopedVector<RegistryEntry> entries;
+
+    base::string16 app_id_shell_key(ShellUtil::kRegClasses);
+    app_id_shell_key.push_back(base::FilePath::kSeparators[0]);
+    app_id_shell_key.append(app_info.app_id);
+    app_id_shell_key.append(ShellUtil::kRegExePath);
+    app_id_shell_key.append(ShellUtil::kRegShellPath);
+
+    // <root hkey>\Software\Classes\<app_id>\.exe\shell @=open
+    entries.push_back(
+        new RegistryEntry(app_id_shell_key, ShellUtil::kRegVerbOpen));
+
+    // The command to execute when opening this application via the Metro UI.
+    const base::string16 delegate_command(
+        ShellUtil::GetChromeDelegateCommand(chrome_exe));
+
+    // Each of Chrome's shortcuts has an appid; which, as of Windows 8, is
+    // registered to handle some verbs. This registration has the side-effect
+    // that these verbs now show up in the shortcut's context menu. We
+    // mitigate this side-effect by making the context menu entries
+    // user readable/localized strings. See relevant MSDN article:
+    // http://msdn.microsoft.com/en-US/library/windows/desktop/cc144171.aspx
+    const struct {
+      const wchar_t* verb;
+      int name_id;
+    } verbs[] = {
+        {ShellUtil::kRegVerbOpen, -1},
+        {ShellUtil::kRegVerbOpenNewWindow, IDS_SHORTCUT_NEW_WINDOW_BASE},
+    };
+    for (size_t i = 0; i < arraysize(verbs); ++i) {

[grt (UTC plus 2), 2015/08/24 15:08:19]

nit: for (const auto verb_and_id& : verbs) {

[gab, 2015/08/27 20:51:07]

Woohoo C++11 in shell_util :-)!

+      base::string16 sub_path(app_id_shell_key);
+      sub_path.push_back(base::FilePath::kSeparators[0]);
+      sub_path.append(verbs[i].verb);
+
+      // <root hkey>\Software\Classes\<app_id>\.exe\shell\<verb>
+      if (verbs[i].name_id != -1) {
+        // TODO(grt): http://crbug.com/75152 Write a reference to a localized
+        // resource.
+        const base::string16 verb_name(
+            installer::GetLocalizedString(verbs[i].name_id));
+        entries.push_back(new RegistryEntry(sub_path, verb_name.c_str()));
+      }
+      entries.push_back(
+          new RegistryEntry(sub_path, L"CommandId", L"Browser.Launch"));
+
+      sub_path.push_back(base::FilePath::kSeparators[0]);
+      sub_path.append(ShellUtil::kRegCommand);
+
+      // <root hkey>\Software\Classes\<app_id>\.exe\shell\<verb>\command
+      entries.push_back(new RegistryEntry(sub_path, delegate_command));
+      entries.push_back(new RegistryEntry(
+          sub_path, ShellUtil::kRegDelegateExecute, app_info.delegate_clsid));
+    }
+
+    return entries.Pass();
+  }
+
   // This method returns a list of all the registry entries that
   // are needed to register this installation's ProgId and AppId.
   // These entries need to be registered in HKLM prior to Win8.
@@ -273,12 +328,7 @@ class RegistryEntry {
     app_info.app_id = ShellUtil::GetBrowserModelId(
         dist, InstallUtil::IsPerUserInstall(chrome_exe));
 
-    // The command to execute when opening this application via the Metro UI.
-    base::string16 delegate_command(
-        ShellUtil::GetChromeDelegateCommand(chrome_exe));
-    bool set_delegate_execute =
-        IsChromeMetroSupported() &&
-        dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid);
+    dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid);
 
     // TODO(grt): http://crbug.com/75152 Write a reference to a localized
     // resource for name, description, and company.
@@ -290,56 +340,25 @@ class RegistryEntry {
 
     GetProgIdEntries(app_info, entries);
 
-    // DelegateExecute ProgId. Needed for Chrome Metro in Windows 8. This is
-    // only needed for registring a web browser, not for general associations.
-    if (set_delegate_execute) {
-      base::string16 model_id_shell(ShellUtil::kRegClasses);
-      model_id_shell.push_back(base::FilePath::kSeparators[0]);
-      model_id_shell.append(app_info.app_id);
-      model_id_shell.append(ShellUtil::kRegExePath);
-      model_id_shell.append(ShellUtil::kRegShellPath);
-
-      // <root hkey>\Software\Classes\<app_id>\.exe\shell @=open
-      entries->push_back(new RegistryEntry(model_id_shell,
-                                           ShellUtil::kRegVerbOpen));
-
-      // Each of Chrome's shortcuts has an appid; which, as of Windows 8, is
-      // registered to handle some verbs. This registration has the side-effect
-      // that these verbs now show up in the shortcut's context menu. We
-      // mitigate this side-effect by making the context menu entries
-      // user readable/localized strings. See relevant MSDN article:
-      // http://msdn.microsoft.com/en-US/library/windows/desktop/cc144171.aspx
-      const struct {
-        const wchar_t* verb;
-        int name_id;
-      } verbs[] = {
-          { ShellUtil::kRegVerbOpen, -1 },
-          { ShellUtil::kRegVerbOpenNewWindow, IDS_SHORTCUT_NEW_WINDOW_BASE },
-      };
-      for (size_t i = 0; i < arraysize(verbs); ++i) {
-        base::string16 sub_path(model_id_shell);
-        sub_path.push_back(base::FilePath::kSeparators[0]);
-        sub_path.append(verbs[i].verb);
-
-        // <root hkey>\Software\Classes\<app_id>\.exe\shell\<verb>
-        if (verbs[i].name_id != -1) {
-          // TODO(grt): http://crbug.com/75152 Write a reference to a localized
-          // resource.
-          base::string16 verb_name(
-              installer::GetLocalizedString(verbs[i].name_id));
-          entries->push_back(new RegistryEntry(sub_path, verb_name.c_str()));
+    if (!app_info.delegate_clsid.empty()) {

[grt (UTC plus 2), 2015/08/24 15:08:19]

    if (dist->GetCommandExecuteImplClsid(&app_info.delegate_clsid)) {
(and remove line 331)

[gab, 2015/08/27 20:51:07]

Done.

[gab, 2015/08/27 21:48:53]

Actually, no, GetProgIdEntries() needs app_info.delegate_clsid.

Which actually makes it more consistent since everybody checks for .empty() --
making the bool return on this GetCommandExecuteImplClsid() unused I think
(shall we clean it up in a follow-up?)

[grt (UTC plus 2), 2015/08/29 01:38:41]

Ah! Subtle!
Yeah. The bool-as-return-value is vestigial. The method once had several out
params. Now, however, it should just return the one string directly.

[gab, 2015/08/29 14:39:30]

Ok will do.

+      ScopedVector<RegistryEntry> delegate_execute_entries =
+          GetChromeDelegateExecuteEntries(chrome_exe, app_info);
+      if (!IsChromeMetroSupported()) {
+        for (RegistryEntry* entry : delegate_execute_entries) {
+          // Need to get rid of the DelegateExecute keys (verbs, etc.)

[grt (UTC plus 2), 2015/08/24 15:08:19]

nit: move the comment out of the loop body so the loop can be just:
        // Remove the keys (not only their values) so that Windows will continue
        // to launch Chrome without a pesky association error.
        for (RegistryEntry* entry : delegate_execute_entries)
          entry->set_removal_flag(RemovalFlag::KEY);

[gab, 2015/08/27 20:51:07]

Done.

+          // themselves (not only their values) or Chrome fails to launch as
+          // Windows finds a registered verb hierarchy with no registered
+          // commands underneath and reports "This file does not have a program
+          // associated with it for performing this action. Please install a
+          // program or, if one is already installed, create an association in
+          // the Default Programs control panel."
+          entry->set_removal_flag(RemovalFlag::KEY);
         }
-        entries->push_back(new RegistryEntry(
-            sub_path, L"CommandId", L"Browser.Launch"));
-
-        sub_path.push_back(base::FilePath::kSeparators[0]);
-        sub_path.append(ShellUtil::kRegCommand);
-
-        // <root hkey>\Software\Classes\<app_id>\.exe\shell\<verb>\command
-        entries->push_back(new RegistryEntry(sub_path, delegate_command));
-        entries->push_back(new RegistryEntry(
-            sub_path, ShellUtil::kRegDelegateExecute, app_info.delegate_clsid));
       }
+      // Move |delegate_execute_entries| to |entries|.
+      entries->insert(entries->end(), delegate_execute_entries.begin(),
+                      delegate_execute_entries.end());
+      delegate_execute_entries.weak_clear();
     }
   }
 
@@ -364,10 +383,13 @@ class RegistryEntry {
     entries->push_back(new RegistryEntry(
         prog_id_path + ShellUtil::kRegShellOpen, app_info.command_line));
     if (!app_info.delegate_clsid.empty()) {
-      entries->push_back(
+      scoped_ptr<RegistryEntry> delegate_execute_entry(
           new RegistryEntry(prog_id_path + ShellUtil::kRegShellOpen,
                             ShellUtil::kRegDelegateExecute,
                             app_info.delegate_clsid));
+      if (!IsChromeMetroSupported())
+        delegate_execute_entry->set_removal_flag(RemovalFlag::VALUE);

[grt (UTC plus 2), 2015/08/24 15:08:20]

do away with the delegate_execute_entry local var and change this to:
        entries->back().set_removal_flag(RemovalFlag::VALUE);

[gab, 2015/08/27 20:51:07]

Done.

+      entries->push_back(delegate_execute_entry.Pass());
     }
 
     // The following entries are required as of Windows 8, but do not
@@ -646,16 +668,35 @@ class RegistryEntry {
     entries->push_back(new RegistryEntry(start_menu, app_name));
   }
 
-  // Generate work_item tasks required to create current registry entry and
-  // add them to the given work item list.
+  // Flags this RegistryKey with |removal_flag|, indicating that it should be
+  // removed rather than created. Note that this will not result in cleaning up
+  // the entire registry hierarchy below RegistryEntry even if it is left empty
+  // by this operation (this should thus not be used for uninstall, but only to
+  // unregister keys that should explicitly no longer be active in the current
+  // configuration).
+  void set_removal_flag(RemovalFlag removal_flag) {
+    removal_flag_ = removal_flag;
+  }
+
+  // Generate work_item tasks required to create (or potentially delete based on

[grt (UTC plus 2), 2015/08/24 15:08:19]

nit: Generates...

[gab, 2015/08/27 20:51:07]

Done.

+  // |removal_flag_|) the current RegistryEntry and add them to the given
+  // work item list.
   void AddToWorkItemList(HKEY root, WorkItemList *items) const {
-    items->AddCreateRegKeyWorkItem(root, key_path_, WorkItem::kWow64Default);
-    if (is_string_) {
-      items->AddSetRegValueWorkItem(
-          root, key_path_, WorkItem::kWow64Default, name_, value_, true);
+    if (removal_flag_ == RemovalFlag::VALUE) {
+      items->AddDeleteRegValueWorkItem(root, key_path_, WorkItem::kWow64Default,
+                                       name_);

[grt (UTC plus 2), 2015/08/24 15:08:20]

this should be best-effort, no?
                                       name_)->set_ignore_failure(true);

[gab, 2015/08/27 20:51:07]

I don't think so, when I was figuring out what should be removed I could easily
get into states where Chrome fails to launch with an error dialog when only some
of the stuff was removed, so we should either make sure it's all gone or all
still there, in betweens are bad.

(if you were talking about the failing if the entry is already not present, this
is already the behavior of the delete work items)

[grt (UTC plus 2), 2015/08/29 01:38:41]

It looks like DeleteRegValueWorkItemTest.DeleteNonExistentValue tests that the
work item succeeds if the value is not present, so that's good. Could you add a
test that the work item succeeds if the containing key is not present? The exact
last-error-code for RegOpenKeyEx when the key doesn't exist isn't documented. If
memory serves, it's ERROR_FILE_NOT_FOUND on some OS versions, and
ERROR_PATH_NOT_FOUND on others.

[gab, 2015/08/29 14:39:30]

Done in follow-up CL @ https://codereview.chromium.org/1309873005/ (preferred
follow-up CL because I also modernized those tests as part of touching them).

ERROR_FILE_NOT_FOUND is what's used in other tests IIRC, pretty sure it passes
on all versions we support, but we can adjust if not.

+    } else if (removal_flag_ == RemovalFlag::KEY) {
+      items->AddDeleteRegKeyWorkItem(root, key_path_, WorkItem::kWow64Default);

[grt (UTC plus 2), 2015/08/24 15:08:19]

and this?

[gab, 2015/08/27 20:51:07]

Same thing.

     } else {
-      items->AddSetRegValueWorkItem(
-          root, key_path_, WorkItem::kWow64Default, name_, int_value_, true);
+      DCHECK(removal_flag_ == RemovalFlag::NONE);
+      items->AddCreateRegKeyWorkItem(root, key_path_, WorkItem::kWow64Default);
+      if (is_string_) {
+        items->AddSetRegValueWorkItem(root, key_path_, WorkItem::kWow64Default,
+                                      name_, value_, true);
+      } else {
+        items->AddSetRegValueWorkItem(root, key_path_, WorkItem::kWow64Default,
+                                      name_, int_value_, true);
+      }
     }
   }
 
@@ -710,23 +751,34 @@ class RegistryEntry {
 
   // Create a object that represent default value of a key
   RegistryEntry(const base::string16& key_path, const base::string16& value)
-      : key_path_(key_path), name_(),
-        is_string_(true), value_(value), int_value_(0) {
-  }
+      : key_path_(key_path),
+        name_(),
+        is_string_(true),
+        value_(value),
+        int_value_(0),
+        removal_flag_(RemovalFlag::NONE) {}
 
   // Create a object that represent a key of type REG_SZ
-  RegistryEntry(const base::string16& key_path, const base::string16& name,
+  RegistryEntry(const base::string16& key_path,
+                const base::string16& name,
                 const base::string16& value)
-      : key_path_(key_path), name_(name),
-        is_string_(true), value_(value), int_value_(0) {
-  }
+      : key_path_(key_path),
+        name_(name),
+        is_string_(true),
+        value_(value),
+        int_value_(0),
+        removal_flag_(RemovalFlag::NONE) {}
 
   // Create a object that represent a key of integer type
-  RegistryEntry(const base::string16& key_path, const base::string16& name,
+  RegistryEntry(const base::string16& key_path,
+                const base::string16& name,
                 DWORD value)
-      : key_path_(key_path), name_(name),
-        is_string_(false), value_(), int_value_(value) {
-  }
+      : key_path_(key_path),
+        name_(name),
+        is_string_(false),
+        value_(),
+        int_value_(value),
+        removal_flag_(RemovalFlag::NONE) {}
 
   base::string16 key_path_;  // key path for the registry entry
   base::string16 name_;      // name of the registry entry
@@ -734,6 +786,10 @@ class RegistryEntry {
   base::string16 value_;     // string value (useful if is_string_ = true)
   DWORD int_value_;    // integer value (useful if is_string_ = false)
 
+  // Identifies whether this RegistryEntry is flagged for removal (i.e. no
+  // longer relevant on the configuration it was created under).
+  RemovalFlag removal_flag_;
+
   // Helper function for ExistsInRegistry().
   // Returns the RegistryStatus of the current registry entry in
   // |root|\|key_path_|\|name_|.
@@ -1155,7 +1211,7 @@ base::win::ShortcutProperties TranslateShortcutProperties(
 // <root>\Software\Classes\Chrome<.suffix>\.exe\shell\run on Windows 8.
 void RemoveRunVerbOnWindows8(BrowserDistribution* dist,
                              const base::FilePath& chrome_exe) {
-  if (IsChromeMetroSupported()) {
+  if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
     bool is_per_user_install = InstallUtil::IsPerUserInstall(chrome_exe);
     HKEY root_key = DetermineRegistrationRoot(is_per_user_install);
     // There's no need to rollback, so forgo the usual work item lists and just