Always suffix ChromeHTML entries on Windows for user-level installs.

chrome/installer/util/shell_util.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.
 //
 // This file defines functions that integrate Chrome in Windows shell. These
 // functions can be used by Chrome as well as Chrome installer. All of the
 // work is done by the local functions defined in anonymous namespace in
 // this class.
 
 #include "chrome/installer/util/shell_util.h"
@@ skipping 56 matching lines @@
   return VerifyVersionInfo(&min_version_info, type_mask, condition_mask) != 0;
 }
 
 // This class represents a single registry entry. The objective is to
 // encapsulate all the registry entries required for registering Chrome at one
 // place. This class can not be instantiated outside the class and the objects
 // of this class type can be obtained only by calling a static method of this
 // class.
 class RegistryEntry {
  public:
+  // A bit-field enum of places to look for this key in the Windows registry.
+  enum LookForIn {
+    LOOK_IN_HKCU = 1 << 0,
+    LOOK_IN_HKLM = 1 << 1,
+  };
+
   // Returns the Windows browser client registration key for Chrome.  For
   // example: "Software\Clients\StartMenuInternet\Chromium[.user]".  Strictly
   // speaking, we should use the name of the executable (e.g., "chrome.exe"),
   // but that ship has sailed.  The cost of switching now is re-prompting users
   // to make Chrome their default browser, which isn't polite.  |suffix| is the
   // user-specific registration suffix; see GetUserSpecificDefaultBrowserSuffix
   // in shell_util.h for details.
   static string16 GetBrowserClientKey(BrowserDistribution* dist,
                                       const string16& suffix) {
     DCHECK(suffix.empty() || suffix[0] == L'.');
     return string16(ShellUtil::kRegStartMenuInternet)
         .append(1, L'\\')
         .append(dist->GetApplicationName())
         .append(suffix);
   }
 
   // Returns the Windows Default Programs capabilities key for Chrome.  For
   // example:
   // "Software\Clients\StartMenuInternet\Chromium[.user]\Capabilities".
   static string16 GetCapabilitiesKey(BrowserDistribution* dist,
                                      const string16& suffix) {
     return GetBrowserClientKey(dist, suffix).append(L"\\Capabilities");
   }
 
+  // Returns true if Chrome is registered with the given |suffix| either in HKCU
+  // or HKLM.
+  // This function only checks for the existence of
+  // Software\Classes\ChromeHTML|suffix| and doesn't otherwise validate the
+  // registration of Chrome.
+  static bool IsChromeRegisteredWithSuffix(const string16& suffix) {
+    DCHECK(!suffix.empty());
+    string16 chrome_html_prog_id(ShellUtil::kRegClasses);
+    chrome_html_prog_id.push_back(FilePath::kSeparators[0]);
+    chrome_html_prog_id.append(ShellUtil::kChromeHTMLProgId);
+    chrome_html_prog_id.append(suffix);
+    return RegistryEntry(chrome_html_prog_id,
+                         ShellUtil::kChromeHTMLProgIdDesc).ExistsInRegistry(
+                             LOOK_IN_HKCU | LOOK_IN_HKLM);
+  }
+
   // This method returns a list of all the registry entries that
   // are needed to register Chromium ProgIds.
   // These entries should be registered in HKCU for user-level installs and in
   // HKLM for system-level installs.
   // TODO (gab): Extract the Windows 8 only registrations out of this function.
   static bool GetProgIdEntries(BrowserDistribution* dist,
                                const string16& chrome_exe,
                                const string16& suffix,
                                std::list<RegistryEntry*>* entries) {
     string16 icon_path(ShellUtil::GetChromeIcon(dist, chrome_exe));
@@ skipping 132 matching lines @@
     // strings.
     entries->push_front(new RegistryEntry(install_info, kReinstallCommand,
         quoted_exe_path + L" --" + ASCIIToWide(switches::kMakeDefaultBrowser)));
     entries->push_front(new RegistryEntry(install_info, L"HideIconsCommand",
         quoted_exe_path + L" --" + ASCIIToWide(switches::kHideIcons)));
     entries->push_front(new RegistryEntry(install_info, L"ShowIconsCommand",
         quoted_exe_path + L" --" + ASCIIToWide(switches::kShowIcons)));
     entries->push_front(new RegistryEntry(install_info, L"IconsVisible", 1));
 
     // Register with Default Programs.
-    string16 app_name(dist->GetApplicationName().append(suffix));
+    string16 reg_app_name(dist->GetApplicationName().append(suffix));
     // Tell Windows where to find Chrome's Default Programs info.
     string16 capabilities(GetCapabilitiesKey(dist, suffix));
     entries->push_front(new RegistryEntry(ShellUtil::kRegRegisteredApplications,
-        app_name, capabilities));
+        reg_app_name, capabilities));
     // Write out Chrome's Default Programs info.
     // TODO(grt): http://crbug.com/75152 Write a reference to a localized
     // resource rather than this.
     entries->push_front(new RegistryEntry(
         capabilities, ShellUtil::kRegApplicationDescription,
         dist->GetLongAppDescription()));
     entries->push_front(new RegistryEntry(
         capabilities, ShellUtil::kRegApplicationIcon, icon_path));
     entries->push_front(new RegistryEntry(
-        capabilities, ShellUtil::kRegApplicationName, app_name));
+        capabilities, ShellUtil::kRegApplicationName,
+        dist->GetApplicationName()));
 
     entries->push_front(new RegistryEntry(capabilities + L"\\Startmenu",
-        L"StartMenuInternet", app_name));
+        L"StartMenuInternet", reg_app_name));
 
     string16 html_prog_id(ShellUtil::kChromeHTMLProgId);
     html_prog_id.append(suffix);
     for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; i++) {
       entries->push_front(new RegistryEntry(
           capabilities + L"\\FileAssociations",
           ShellUtil::kFileAssociations[i], html_prog_id));
     }
     for (int i = 0; ShellUtil::kPotentialProtocolAssociations[i] != NULL;
         i++) {
@@ skipping 89 matching lines @@
     if (_is_string) {
       items->AddSetRegValueWorkItem(root, _key_path, _name, _value, true);
     } else {
       items->AddSetRegValueWorkItem(root, _key_path, _name, _int_value, true);
     }
   }
 
   // Checks if the current registry entry exists in HKCU\|_key_path|\|_name|
   // and value is |_value|. If the key does NOT exist in HKCU, checks for
   // the correct name and value in HKLM.
-  // This mimics Windows' behavior when searching in HKCR (HKCU takes precedence
-  // over HKLM). For registrations outside of HKCR on versions of Windows up
-  // to Win7, Chrome's values go in HKLM. This function will make unnecessary
-  // (but harmless) queries into HKCU in that case. Starting with Windows 8,
-  // Chrome's values go in HKCU for user-level installs, which takes precedence
-  // over HKLM.
-  bool ExistsInRegistry() {
-    RegistryStatus hkcu_status = StatusInRegistryUnderRoot(HKEY_CURRENT_USER);
+  // |look_for_in| specifies roots (HKCU and/or HKLM) in which to look for the
+  // key, unspecified roots are not looked in and the key is assumed not to
+  // exist in them.
+  // |look_for_in| must at least specify one root to look into.
+  // If |look_for_in| specifies both HKCU and HKLM, this method mimics Windows'
+  // behavior when searching in HKCR (HKCU takes precedence over HKLM). For
+  // registrations outside of HKCR on versions of Windows up to Win7, Chrome's
+  // values go in HKLM. This function will make unnecessary (but harmless)
+  // queries into HKCU in that case. Starting with Windows 8, Chrome's values go

[grt (UTC plus 2), 2012/05/30 20:37:40]

Is this statement about where Chrome's values go in Windows 8 true?  I thought
this was blocked on a perceived bug in Windows.

[gab, 2012/05/31 06:36:01]

Yes, in fact it was already checked in somehow, I guess I previously wrote that
dreaming of a better future :)!

I'll remove it for now as that's not the current behavior.

+  // in HKCU for user-level installs, which takes precedence over HKLM.
+  bool ExistsInRegistry(uint32 look_for_in) const {
+    DCHECK(look_for_in);
+
+    RegistryStatus hkcu_status = DOES_NOT_EXIST;
+    if (look_for_in & LOOK_IN_HKCU)
+      hkcu_status = StatusInRegistryUnderRoot(HKEY_CURRENT_USER);
+
+    RegistryStatus hklm_status = DOES_NOT_EXIST;
+    if (hkcu_status == DOES_NOT_EXIST && (look_for_in & LOOK_IN_HKLM))
+      hklm_status = StatusInRegistryUnderRoot(HKEY_LOCAL_MACHINE);
+
     return (hkcu_status == SAME_VALUE ||
-            (hkcu_status == DOES_NOT_EXIST &&
-             StatusInRegistryUnderRoot(HKEY_LOCAL_MACHINE) == SAME_VALUE));
+            (hkcu_status == DOES_NOT_EXIST && hklm_status == SAME_VALUE));
   }
 
  private:
   // States this RegistryKey can be in compared to the registry.
   enum RegistryStatus {
     // |_name| does not exist in the registry
     DOES_NOT_EXIST,
     // |_name| exists, but its value != |_value|
     DIFFERENT_VALUE,
     // |_name| exists and its value is |_value|
@@ skipping 63 matching lines @@
 
   // Apply all the registry changes and if there is a problem, rollback
   if (!items->Do()) {
     items->Rollback();
     return false;
   }
   return true;
 }
 
 // Checks that all |entries| are present on this computer.
-bool AreEntriesRegistered(const std::list<RegistryEntry*>& entries) {
+// |look_for_in| is passed to RegistryEntry::ExistsInRegistry(). Documentation
+// for it can be found there.
+bool AreEntriesRegistered(const std::list<RegistryEntry*>& entries,
+                          uint32 look_for_in) {
   bool registered = true;
   for (std::list<RegistryEntry*>::const_iterator itr = entries.begin();
        registered && itr != entries.end(); ++itr) {
     // We do not need registered = registered && ... since the loop condition
     // is set to exit early.
-    registered = (*itr)->ExistsInRegistry();
+    registered = (*itr)->ExistsInRegistry(look_for_in);
   }
   return registered;
 }
 
 // Checks that all required registry entries for Chrome are already present
 // on this computer.
 bool IsChromeRegistered(BrowserDistribution* dist,
                         const string16& chrome_exe,
                         const string16& suffix) {
   std::list<RegistryEntry*> entries;
   STLElementDeleter<std::list<RegistryEntry*> > entries_deleter(&entries);
   RegistryEntry::GetProgIdEntries(dist, chrome_exe, suffix, &entries);
   RegistryEntry::GetSystemEntries(dist, chrome_exe, suffix, &entries);
-  return AreEntriesRegistered(entries);
+  return AreEntriesRegistered(
+      entries, RegistryEntry::LOOK_IN_HKCU | RegistryEntry::LOOK_IN_HKLM);
 }
 
 // This method checks if Chrome is already registered on the local machine
 // for the requested protocol. It just checks the one value required for this.
 bool IsChromeRegisteredForProtocol(BrowserDistribution* dist,
                                    const string16& suffix,
                                    const string16& protocol) {
   std::list<RegistryEntry*> entries;
   STLElementDeleter<std::list<RegistryEntry*> > entries_deleter(&entries);
   RegistryEntry::GetProtocolCapabilityEntries(dist, suffix, protocol, &entries);
-  return AreEntriesRegistered(entries);
+  return AreEntriesRegistered(
+      entries, RegistryEntry::LOOK_IN_HKCU | RegistryEntry::LOOK_IN_HKLM);
 }
 
 // This method registers Chrome on Vista by launching an elevated setup.exe.
 // That will show the user the standard Vista elevation prompt. If the user
 // accepts it the new process will make the necessary changes and return SUCCESS
 // that we capture and return.
 // If protocol is non-empty we will also register Chrome as being capable of
 // handling the protocol.
 bool ElevateAndRegisterChrome(BrowserDistribution* dist,
                               const string16& chrome_exe,
@@ skipping 32 matching lines @@
     }
 
     DWORD ret_val = 0;
     InstallUtil::ExecuteExeAsAdmin(cmd, &ret_val);
     if (ret_val == 0)
       return true;
   }
   return false;
 }
 
-// This method tries to figure out if another user has already registered her
-// own copy of Chrome so that we can avoid overwriting it and append current
-// user's login name to default browser registry entries. This function is
-// not meant to detect all cases. It just tries to handle the most common case.
-// All the conditions below have to be true for it to return true:
-// - Software\Clients\StartMenuInternet\Chromium\"" key should have a valid
-//   value.
-// - The value should not be same as given value in |chrome_exe|
-// - Finally to handle the default install path (C:\Document and Settings\
-//   <user>\Local Settings\Application Data\Chromium\Application) the value
-//   of the above key should differ from |chrome_exe| only in user name.
-bool AnotherUserHasDefaultBrowser(BrowserDistribution* dist,
-                                  const string16& chrome_exe) {
-  const string16 reg_key(
-      RegistryEntry::GetBrowserClientKey(dist, string16())
-      .append(ShellUtil::kRegShellOpen));
-  RegKey key(HKEY_LOCAL_MACHINE, reg_key.c_str(), KEY_READ);
-  string16 registry_chrome_exe;
-  if ((key.ReadValue(L"", &registry_chrome_exe) != ERROR_SUCCESS) ||
-      registry_chrome_exe.length() < 2)
-    return false;
-
-  registry_chrome_exe = registry_chrome_exe.substr(1,
-      registry_chrome_exe.length() - 2);
-  if ((registry_chrome_exe.size() == chrome_exe.size()) &&
-      (std::equal(chrome_exe.begin(), chrome_exe.end(),
-                  registry_chrome_exe.begin(),
-                  base::CaseInsensitiveCompare<wchar_t>()))) {
-    return false;
-  }
-
-  std::vector<string16> v1, v2;
-  base::SplitString(registry_chrome_exe, L'\\', &v1);
-  base::SplitString(chrome_exe, L'\\', &v2);
-  if (v1.empty() || v2.empty() || v1.size() != v2.size())
-    return false;
-
-  // Now check that only one of the values within two '\' chars differ.
-  std::vector<string16>::iterator itr1 = v1.begin();
-  std::vector<string16>::iterator itr2 = v2.begin();
-  bool one_mismatch = false;
-  for ( ; itr1 < v1.end() && itr2 < v2.end(); ++itr1, ++itr2) {
-    string16 s1 = *itr1;
-    string16 s2 = *itr2;
-    if ((s1.size() != s2.size()) ||
-        (!std::equal(s1.begin(), s1.end(),
-                     s2.begin(), base::CaseInsensitiveCompare<wchar_t>()))) {
-      if (one_mismatch)
-        return false;
-      else
-        one_mismatch = true;
-    }
-  }
-  return true;
-}
-
 // Launches the Windows 7 and Windows 8 application association dialog, which
 // is the only documented way to make a browser the default browser on
 // Windows 8.
 bool LaunchApplicationAssociationDialog(const string16& app_id) {
   base::win::ScopedComPtr<IApplicationAssociationRegistrationUI> aarui;
   HRESULT hr = aarui.CreateInstance(CLSID_ApplicationAssociationRegistrationUI);
   if (FAILED(hr))
     return false;
   hr = aarui->LaunchAdvancedAssociationUI(app_id.c_str());
   return SUCCEEDED(hr);
@@ skipping 16 matching lines @@
 // should be removed after a reasonable time, say 2012-08-01.  Anyone on Win8
 // dev channel who hasn't been autoupdated or manually updated by then will have
 // to uninstall and reinstall Chrome to repair.  See http://crbug.com/124666 and
 // http://crbug.com/123994 for gory details.
 // This function is also used to remove DelegateExecute verb handler
 // registrations on builds for which Metro is no longer supported. This will
 // also become irrelevant sometime after Windows 8 RC (thus the aforementioned
 // removal date remains correct).
 void RemoveBadWindows8RegistrationIfNeeded(
     BrowserDistribution* dist,
-    const string16& chrome_exe,
-    const string16& suffix) {
+    const string16& chrome_exe) {
   string16 handler_guid;
 
   if (dist->GetDelegateExecuteHandlerData(&handler_guid, NULL, NULL, NULL) &&
       (!InstallUtil::HasDelegateExecuteHandler(dist, chrome_exe) ||
        !IsChromeMetroSupported())) {
     // There's no need to rollback, so forgo the usual work item lists and just
     // remove the values from the registry.
     const HKEY root_key = InstallUtil::IsPerUserInstall(chrome_exe.c_str()) ?
         HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE;
+    // Use the current installation's suffix, not the about-to-be-installed
+    // suffix.
+    const string16 installation_suffix =
+        ShellUtil::GetCurrentInstallationSuffix();
     const string16 app_id(dist->GetBrowserAppId());
 
     // <root hkey>\Software\Classes\<app_id>
     string16 key(ShellUtil::kRegClasses);
     key.push_back(FilePath::kSeparators[0]);
     key.append(app_id);
     InstallUtil::DeleteRegistryKey(root_key, key);
 
     // <root hkey>\Software\Classes\ChromiumHTML[.user]\shell\open\command
     key = ShellUtil::kRegClasses;
     key.push_back(FilePath::kSeparators[0]);
     key.append(ShellUtil::kChromeHTMLProgId);
-    key.append(suffix);
+    key.append(installation_suffix);
     key.append(ShellUtil::kRegShellOpen);
     InstallUtil::DeleteRegistryValue(root_key, key,
                                      ShellUtil::kRegDelegateExecute);
   }
 }
 
+// Returns true if the current user-level install has been registered without
+// a suffix. This is used to support old installs where we used to not suffix
+// user-level installs if they were the first to request the non-suffixed
+// registry entries on the machine. This however created clashes with
+// system-level installs when updating user-level installs to system-level
+// installs. There is no downside to suffixing anyways (the name registered is
+// not user visible).
+// All the conditions below have to be true for it to return true:
+// - Software\Clients\StartMenuInternet\Chromium\"" key should have a valid
+//   value.
+// - The value should be same as given value in |chrome_exe|.
+bool CurrentUserIsRegisteredWithNoSuffix(BrowserDistribution* dist,
+                                       const string16& chrome_exe) {

[grt (UTC plus 2), 2012/05/30 20:37:40]

nit: indentation

[gab, 2012/05/31 06:36:01]

Done.

+  // Assert that a Chrome is registered under
+  // HKLM\Software\Clients\StartMenuInternet\Google Chrome
+  const string16 reg_key(
+      RegistryEntry::GetBrowserClientKey(dist, string16())
+      .append(ShellUtil::kRegShellOpen));
+  RegKey key(HKEY_LOCAL_MACHINE, reg_key.c_str(), KEY_READ);
+  string16 registry_chrome_exe;
+  if ((key.ReadValue(L"", &registry_chrome_exe) != ERROR_SUCCESS) ||
+      registry_chrome_exe.length() < 2) {
+    return false;
+  }
+
+  // Assert that the Chrome registered there is |chrome_exe| (i.e. this user's

[grt (UTC plus 2), 2012/05/30 20:37:40]

String comparisons of paths make me uncomfortable.  setup_util's ProgramCompare
will do a more thorough check.  Feel free to move it into installer_util so it
can be reused.  This function would be shorter using that, too.

[gab, 2012/05/31 06:36:01]

This code was copied from the old AnotherUserHasDefaultBrowser() removed above.
I agree that ProgramCompare is much better, I'll move it as requested
(http://codereview.chromium.org/10446095/) and use it here.

+  // Chrome).
+  registry_chrome_exe = registry_chrome_exe.substr(1,
+      registry_chrome_exe.length() - 2);
+  if (registry_chrome_exe.size() != chrome_exe.size() ||
+      !std::equal(chrome_exe.begin(), chrome_exe.end(),
+                  registry_chrome_exe.begin(),
+                  base::CaseInsensitiveCompare<wchar_t>())) {
+    return false;
+  }
+
+  return true;
+}
+
+// Sets |suffix| to this user's username preceded by a dot. This suffix is then
+// meant to be added to all registration that may conflict with another
+// user-level Chrome install.
+// Returns true unless the OS call to retrieve the username fails.
+bool GetUserSpecificRegistrySuffix(string16* suffix) {
+  wchar_t user_name[256];
+  DWORD size = arraysize(user_name);
+  if (::GetUserName(user_name, &size) == 0 || size < 1) {
+    LOG(DFATAL) << "Failed to get username in " << __FUNCTION__ << ".";

[grt (UTC plus 2), 2012/05/30 20:37:40]

I think you can remove __FUNCTION__ since it doesn't add anything to the log
message.  The string is enough to find the line in the source code.  Also, use
PLOG rather than LOG so that the last error code is automagically logged.

[gab, 2012/05/31 06:36:01]

Ok, my thinking was that it's quite a generic string and this makes it specific,
but I guess there still can't be more than a couple (if any) duplicate of this
string in the code...

[grt (UTC plus 2), 2012/06/04 02:18:26]

I find __FUNCTION__ can be really helpful in tests that generate lots of output.
 In this case, though I don't think it's needed since file and line are logged.

[gab, 2012/06/05 19:50:02]

Ah ok, hadn't realized file and line were logged, I agree then.

Thanks for clarifying!

+    return false;
+  }
+  suffix->reserve(size);
+  suffix->assign(1, L'.');
+  suffix->append(user_name, size - 1);
+  return true;
+}
+
+// Sets |suffix| to the current user's username on user-level installs.

[grt (UTC plus 2), 2012/05/30 20:37:40]

Regarding "to the current user's username": please mention that it's preceded by
'.'.

[gab, 2012/05/31 06:36:01]

Done.

+// To support old-style user-level installs however, |suffix| is cleared if
+// the user currently owns the non-suffixed HKLM registrations.
+// |suffix| is also cleared on system-level installs.
+// |suffix| should then be appended to all Chrome properties that may conflict
+// with other Chrome user-level installs.
+// Returns true unless one of the underlying calls fails.
+bool GetInstallationSpecificSuffix(BrowserDistribution* dist,
+                                   const string16& chrome_exe,
+                                   string16* suffix) {
+  if (!InstallUtil::IsPerUserInstall(chrome_exe.c_str()) ||
+      CurrentUserIsRegisteredWithNoSuffix(dist, chrome_exe)) {
+    // No suffix on system-level installs and user-level installs already
+    // registered with no suffix.
+    *suffix = L"";
+    return true;
+  } else {
+    return GetUserSpecificRegistrySuffix(suffix);
+  }
+}
+
 }  // namespace
 
 const wchar_t* ShellUtil::kRegDefaultIcon = L"\\DefaultIcon";
 const wchar_t* ShellUtil::kRegShellPath = L"\\shell";
 const wchar_t* ShellUtil::kRegShellOpen = L"\\shell\\open\\command";
 const wchar_t* ShellUtil::kRegStartMenuInternet =
     L"Software\\Clients\\StartMenuInternet";
 const wchar_t* ShellUtil::kRegClasses = L"Software\\Classes";
 const wchar_t* ShellUtil::kRegRegisteredApplications =
     L"Software\\RegisteredApplications";
@@ skipping 226 matching lines @@
                  KEY_QUERY_VALUE) != ERROR_SUCCESS ||
         key.ReadValue(kReinstallCommand, &command) != ERROR_SUCCESS) {
       continue;
     }
     if (!name.empty() && !command.empty() &&
         name.find(dist->GetApplicationName()) == string16::npos)
       (*browsers)[name] = command;
   }
 }
 
-bool ShellUtil::GetUserSpecificDefaultBrowserSuffix(BrowserDistribution* dist,
-                                                    string16* entry) {
-  wchar_t user_name[256];
-  DWORD size = arraysize(user_name);
-  if (::GetUserName(user_name, &size) == 0 || size < 1)
-    return false;
-  entry->reserve(size);
-  entry->assign(1, L'.');
-  entry->append(user_name, size - 1);
+string16 ShellUtil::GetCurrentInstallationSuffix() {
+  // Make sure we return an empty suffix if this is a system-level install.
+  // This check is needed because it is technically possible for a system-level
+  // install to run when a suffixed user-level is present (if the user never
+  // runs his user-level install after a system-level Chrome is installed and
+  // instead runs the system-level chrome himself from Program Files...). In the
+  // unlikely event that we cannot get the path to chrome exe however, skip
+  // this check as it is already checking for a corner case...
+  FilePath chrome_exe;
+  bool skip_system_install_check = false;
+  if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {

[grt (UTC plus 2), 2012/05/30 20:37:40]

In general, we should strive to not do this PathService and IsPerUserInstall
stuff deep in utility functions.  The main reasons are that they both
potentially do real work (i.e., I/O) and they make testing very difficult.  It
looks like all callers in this file already have the path to chrome, so it
should be easy to change the signature to take const FilePath& chrome_exe.  The
call to IsPerUserInstall could potentially stay if needed, since passing in the
path is enough to make tests feasible.

[gab, 2012/05/31 06:36:01]

Right, I don't like it either, the problem though is that this is not called at
install-time (where we know chrome_exe ), but by various callers at run-time
(that don't know chrome_exe).

In the end though this is all just to make sure this is not a  system-level
install for a corner case described in the comment...

I see other ways of doing this, but they aren't really better:
- Singleton with chrome's install state
- Caching of the suffix (I don't know if Chromium prefers CPU overload to memory
overload (i.e. is caching usually a good thing for this kind of function?))...
this addresses the "repetitive I/O" problem, but doesn't address the testing
problem...

[grt (UTC plus 2), 2012/06/04 02:18:26]

My suggestion was to change the function signature so that const FilePath&
chrome_exe was passed in.  This is simple for all callers in this file since
they already have the path to chrome_exe.  It's also simple for callers in
uninstall.cc since they know the path to chrome.exe (and using PathService will
actually give them the path to setup.exe anyway).  Any other callers from within
Chrome can use PathService themselves.

[gab, 2012/06/05 19:50:02]

Ah ok yes, makes sense.

Done.

+    LOG(DFATAL) << "Failed to get the path to chrome exe in " << __FUNCTION__;
+    skip_system_install_check = true;
+  }
 
-  return RegKey(HKEY_LOCAL_MACHINE,
-                RegistryEntry::GetBrowserClientKey(dist, *entry).c_str(),
-                KEY_READ).Valid();
+  string16 tested_suffix;
+  if ((!skip_system_install_check &&
+       !InstallUtil::IsPerUserInstall(chrome_exe.value().c_str())) ||
+      !GetUserSpecificRegistrySuffix(&tested_suffix) ||
+      !RegistryEntry::IsChromeRegisteredWithSuffix(tested_suffix)) {

[grt (UTC plus 2), 2012/05/30 20:37:40]

What's the motivation for looking at the ProgID rather than the client key (as
is done by CurrentUserIsRegisteredWithNoSuffix)?

[gab, 2012/05/31 06:36:01]

Chrome can have only its ProgIds registered (if the user is not admin on Win7-).

This function is trying to determine the suffix of the current Chrome for
run-time behavior.

CurrentUserIsRegisteredWithNoSuffix is trying to determine whether, at
update-time, we should update the registry without a suffix. This call on the
other hand can't be replaced by
!RegistryEntry::IsChromeRegisteredWithSuffix(suffix), as we want to always
suffix UNLESS a no suffix install IS present (i.e. not not suffix if no suffix
install is present...).

+    return L"";
+  }
+  return tested_suffix;
+}
+
+bool ShellUtil::IsInstallationPresentInHKLM(BrowserDistribution* dist,
+                                            const string16& chrome_exe,
+                                            const string16& suffix) {
+  std::list<RegistryEntry*> entries;
+  STLElementDeleter<std::list<RegistryEntry*> > entries_deleter(&entries);
+  RegistryEntry::GetSystemEntries(dist, chrome_exe, suffix, &entries);
+  return AreEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKLM);
 }
 
 bool ShellUtil::MakeChromeDefault(BrowserDistribution* dist,
                                   int shell_change,
                                   const string16& chrome_exe,
                                   bool elevate_if_not_admin) {
   if (!dist->CanSetAsDefault())
     return false;
 
   ShellUtil::RegisterChromeBrowser(dist, chrome_exe, L"", elevate_if_not_admin);

[grt (UTC plus 2), 2012/05/30 20:37:40]

Can anything good happen in MakeChromeDefault if RegisterChromeBrowser returns
false?  If not, how about an early exit here?

[gab, 2012/05/31 06:36:01]

Done.

 
   bool ret = true;
   // First use the new "recommended" way on Vista to make Chrome default
   // browser.
   string16 app_name = dist->GetApplicationName();
-  string16 app_suffix;
-  if (ShellUtil::GetUserSpecificDefaultBrowserSuffix(dist, &app_suffix))
-    app_name += app_suffix;
+  const string16 app_suffix(ShellUtil::GetCurrentInstallationSuffix());
+  app_name += app_suffix;
 
   if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
     // On Windows 8, you can't set yourself as the default handler
     // programatically. In other words IApplicationAssociationRegistration
     // has been rendered useless. What you can do is to launch
     // "Set Program Associations" section of the "Default Programs"
     // control panel. This action does not require elevation and we
     // don't get to control window activation. More info at:
     // http://msdn.microsoft.com/en-us/library/cc144154(VS.85).aspx
     return LaunchApplicationAssociationDialog(app_name.c_str());
@@ skipping 29 matching lines @@
     }
   }
 
   // Now use the old way to associate Chrome with supported protocols and file
   // associations. This should not be required on Vista but since some
   // applications still read Software\Classes\http key directly, we have to do
   // this on Vista also.
 
   std::list<RegistryEntry*> entries;
   STLElementDeleter<std::list<RegistryEntry*> > entries_deleter(&entries);
-  string16 suffix;
-  if (!GetUserSpecificDefaultBrowserSuffix(dist, &suffix))
-    suffix = L"";
-  RegistryEntry::GetUserEntries(dist, chrome_exe, suffix, &entries);
+  RegistryEntry::GetUserEntries(dist, chrome_exe, app_suffix, &entries);
   // Change the default browser for current user.
   if ((shell_change & ShellUtil::CURRENT_USER) &&
       !AddRegistryEntries(HKEY_CURRENT_USER, entries)) {
     ret = false;
     LOG(ERROR) << "Could not make Chrome default browser (XP/current user).";
   }
 
   // Chrome as default browser at system level.
   if ((shell_change & ShellUtil::SYSTEM_LEVEL) &&
       !AddRegistryEntries(HKEY_LOCAL_MACHINE, entries)) {
@@ skipping 19 matching lines @@
   // First use the new "recommended" way on Vista to make Chrome default
   // protocol handler.
   if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
     VLOG(1) << "Registering Chrome as default handler for " << protocol
             << " on Vista.";
     base::win::ScopedComPtr<IApplicationAssociationRegistration> pAAR;
     HRESULT hr = pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration,
       NULL, CLSCTX_INPROC);
     if (SUCCEEDED(hr)) {
       string16 app_name = dist->GetApplicationName();
-      string16 suffix;
-      if (ShellUtil::GetUserSpecificDefaultBrowserSuffix(dist, &suffix))
-        app_name += suffix;
+      app_name += ShellUtil::GetCurrentInstallationSuffix();
 
       hr = pAAR->SetAppAsDefault(app_name.c_str(), protocol.c_str(),
                                  AT_URLPROTOCOL);
     }
     if (!SUCCEEDED(hr)) {
       ret = false;
       LOG(ERROR) << "Could not make Chrome default protocol client (Vista):"
                  << " HRESULT=" << hr << ".";
     }
   }
 
   // Now use the old way to associate Chrome with the desired protocol. This
   // should not be required on Vista but since some applications still read
   // Software\Classes\http key directly, we have to do this on Vista also.
 
   std::list<RegistryEntry*> entries;
   STLElementDeleter<std::list<RegistryEntry*> > entries_deleter(&entries);
-  string16 suffix;
-  if (!GetUserSpecificDefaultBrowserSuffix(dist, &suffix))
-    suffix = L"";
-  string16 chrome_open = ShellUtil::GetChromeShellOpenCmd(chrome_exe);
-  string16 chrome_icon = ShellUtil::GetChromeIcon(dist, chrome_exe);
+  const string16 suffix(ShellUtil::GetCurrentInstallationSuffix());
+  const string16 chrome_open(ShellUtil::GetChromeShellOpenCmd(chrome_exe));
+  const string16 chrome_icon(ShellUtil::GetChromeIcon(dist, chrome_exe));
   RegistryEntry::GetUserProtocolEntries(protocol, chrome_icon, chrome_open,
                                         &entries);
   // Change the default protocol handler for current user.
   if (!AddRegistryEntries(HKEY_CURRENT_USER, entries)) {
       ret = false;
       LOG(ERROR) << "Could not make Chrome default protocol client (XP).";
   }
 
   return ret;
 }
 
 bool ShellUtil::RegisterChromeBrowser(BrowserDistribution* dist,
                                       const string16& chrome_exe,
                                       const string16& unique_suffix,
                                       bool elevate_if_not_admin) {
   if (!dist->CanSetAsDefault())
     return false;
 
-  // First figure out we need to append a suffix to the registry entries to
-  // make them unique.
   string16 suffix;
   if (!unique_suffix.empty()) {
     suffix = unique_suffix;
-  } else if (InstallUtil::IsPerUserInstall(chrome_exe.c_str()) &&
-             !GetUserSpecificDefaultBrowserSuffix(dist, &suffix) &&
-             !AnotherUserHasDefaultBrowser(dist, chrome_exe)) {
-    suffix = L"";
+  } else if (!GetInstallationSpecificSuffix(dist, chrome_exe, &suffix)) {
+    return false;
   }
 
   // TODO(grt): remove this on or after 2012-08-01; see impl for details.
-  RemoveBadWindows8RegistrationIfNeeded(dist, chrome_exe, suffix);
+  RemoveBadWindows8RegistrationIfNeeded(dist, chrome_exe);
 
   // Check if Chromium is already registered with this suffix.
   if (IsChromeRegistered(dist, chrome_exe, suffix))
     return true;
 
   // If user is an admin try to register and return the status.
   if (IsUserAnAdmin()) {
     std::list<RegistryEntry*> progids;
     STLElementDeleter<std::list<RegistryEntry*> > progids_deleter(&progids);
     std::list<RegistryEntry*> sys_entries;
     STLElementDeleter<std::list<RegistryEntry*> > sys_deleter(&sys_entries);
     RegistryEntry::GetProgIdEntries(dist, chrome_exe, suffix, &progids);
     RegistryEntry::GetSystemEntries(dist, chrome_exe, suffix, &sys_entries);
     return AddRegistryEntries(
                InstallUtil::IsPerUserInstall(chrome_exe.c_str()) ?
                    HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE,
                progids) &&
            AddRegistryEntries(HKEY_LOCAL_MACHINE, sys_entries);
   }
 
   // If user is not an admin and OS is Vista, try to elevate and register.
   if (elevate_if_not_admin &&
       base::win::GetVersion() >= base::win::VERSION_VISTA &&
       ElevateAndRegisterChrome(dist, chrome_exe, suffix, L""))
     return true;
 
-  // If we got to this point then all we can do is create ProgIds under HKCU
-  // on XP as well as Vista.
+  // If we got to this point then all we can do is create ProgIds under HKCU.
   std::list<RegistryEntry*> entries;
   STLElementDeleter<std::list<RegistryEntry*> > entries_deleter(&entries);
   RegistryEntry::GetProgIdEntries(dist, chrome_exe, L"", &entries);
+  // Prefer to use |suffix|; unless Chrome's ProgIds are already registered with
+  // no suffix (as per the old registration style): in which case some other
+  // registry entries could refer to them and since we were not able to set our
+  // HKLM entries above, we are better off not altering these here.
+  if (!suffix.empty() &&
+      !AreEntriesRegistered(entries, RegistryEntry::LOOK_IN_HKCU)) {
+    entries.clear();
+    RegistryEntry::GetProgIdEntries(dist, chrome_exe, suffix, &entries);
+  }
   return AddRegistryEntries(HKEY_CURRENT_USER, entries);
 }
 
 bool ShellUtil::RegisterChromeForProtocol(BrowserDistribution* dist,
                                           const string16& chrome_exe,
                                           const string16& unique_suffix,
                                           const string16& protocol,
                                           bool elevate_if_not_admin) {
   if (!dist->CanSetAsDefault())
     return false;
 
-  // Figure out we need to append a suffix to the registry entries to
-  // make them unique.
   string16 suffix;
   if (!unique_suffix.empty()) {
     suffix = unique_suffix;
-  } else if (InstallUtil::IsPerUserInstall(chrome_exe.c_str()) &&
-             !GetUserSpecificDefaultBrowserSuffix(dist, &suffix) &&
-             !AnotherUserHasDefaultBrowser(dist, chrome_exe)) {
-    suffix = L"";
+  } else if (!GetInstallationSpecificSuffix(dist, chrome_exe, &suffix)) {
+    return false;
   }
 
   // Check if Chromium is already registered with this suffix.
   if (IsChromeRegisteredForProtocol(dist, suffix, protocol))
     return true;
 
   if (IsUserAnAdmin()) {
     // We can do this operation directly.
     // If we're not registered at all, try to register. If that fails
     // we should give up.
@@ skipping 125 matching lines @@
       chrome_exe.c_str(),
       shortcut.c_str(),
       chrome_path.c_str(),
       arguments.c_str(),
       description.c_str(),
       icon_path.c_str(),
       icon_index,
       dist->GetBrowserAppId().c_str(),
       ConvertShellUtilShortcutOptionsToFileUtil(options));
 }