Clarify function names for registry magic in shell_util.cc

Clarify function names for registry magic in shell_util.cc

After Gangnam Style, XPStyle :).

BUG=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162456

[gab, 2012-10-05 19:16:53]

Simple name changes to clarify methods in shell_util.cc; discovered they were
confusing as I was putting together
https://sites.google.com/a/chromium.org/dev/developers/installer

Cheers,
Gab

[grt (UTC plus 2), 2012-10-06 01:16:24]

http://codereview.chromium.org/11066047/diff/1/chrome/installer/util/shell_ut...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/11066047/diff/1/chrome/installer/util/shell_ut...
chrome/installer/util/shell_util.cc:429: static void
GetUserProtocolEntriesForXP(
in general, i really like this change.  the name gives me pause, though, since
at first glance it's easy to interpret "...ForXP" as meaning that this should
only be used on XP (which isn't the case).  what do you think of
GetXPStyleUserProtocolEntries or GetXPEraUserProtocolEntries or something like
that?

[gab, 2012-10-09 14:14:52]

After gangnam style, XPStyle :)!

http://codereview.chromium.org/11066047/diff/1/chrome/installer/util/shell_ut...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/11066047/diff/1/chrome/installer/util/shell_ut...
chrome/installer/util/shell_util.cc:429: static void
GetUserProtocolEntriesForXP(
On 2012/10/06 01:16:24, grt wrote:
> in general, i really like this change.  the name gives me pause, though, since
> at first glance it's easy to interpret "...ForXP" as meaning that this should
> only be used on XP (which isn't the case).  what do you think of
> GetXPStyleUserProtocolEntries or GetXPEraUserProtocolEntries or something like
> that?

Right, I was going to go with "XPStyle" instead of "ForXP" when I first
introduced this method. Robert didn't like "XPStyle" (I don't recall why), going
back to it now as I agree with you "ForXP" implies this should only be done on
XP. As for "XPEra" it sounds good in the getter ("GetXPEra..."), but not in the
methods ("RegisterChromeAsDefaultXPEra"...), so I'd rather keep it consistent
with "XPStyle".

[grt (UTC plus 2), 2012-10-09 14:35:56]

lgtm

[commit-bot: I haz the power, 2012-10-17 02:49:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11066047/12001

[commit-bot: I haz the power, 2012-10-17 02:49:24]

Failed to apply patch for chrome/installer/util/shell_util.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/installer/util/shell_util.cc
  Hunk #1 succeeded at 425 (offset -1 lines).
  Hunk #2 succeeded at 460 (offset -1 lines).
  Hunk #3 succeeded at 482 (offset -1 lines).
  Hunk #4 succeeded at 882 (offset -1 lines).
  Hunk #5 succeeded at 893 (offset -1 lines).
  Hunk #6 succeeded at 923 (offset -1 lines).
  Hunk #7 succeeded at 1597 (offset 23 lines).
  Hunk #8 FAILED at 1602.
  Hunk #9 succeeded at 1667 (offset 23 lines).
  Hunk #10 FAILED at 1673.
  Hunk #11 succeeded at 1726 (offset 26 lines).
  Hunk #12 succeeded at 1798 (offset 26 lines).
  2 out of 12 hunks FAILED -- saving rejects to file
chrome/installer/util/shell_util.cc.rej

Patch:       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
fd1c0d8e61dfeb468135a7947715fc7d5d24ed70..91b0acde573fe68cb30e78bc8ba253e98752f8ef
100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -426,11 +426,12 @@ class RegistryEntry {
   }
 
   // This method returns a list of all the user level registry entries that
-  // are needed to make Chromium the default handler for a protocol.
-  static void GetUserProtocolEntries(const string16& protocol,
-                                     const string16& chrome_icon,
-                                     const string16& chrome_open,
-                                     ScopedVector<RegistryEntry>* entries) {
+  // are needed to make Chromium the default handler for a protocol on XP.
+  static void GetXPStyleUserProtocolEntries(
+      const string16& protocol,
+      const string16& chrome_icon,
+      const string16& chrome_open,
+      ScopedVector<RegistryEntry>* entries) {
     // Protocols associations.
     string16 url_key(ShellUtil::kRegClasses);
     url_key.push_back(FilePath::kSeparators[0]);
@@ -460,11 +461,11 @@ class RegistryEntry {
   }
 
   // This method returns a list of all the user level registry entries that
-  // are needed to make Chromium default browser.
+  // are needed to make Chromium default browser on XP.
   // Some of these entries are irrelevant in recent versions of Windows, but
   // we register them anyways as some legacy apps are hardcoded to lookup those
   // values.
-  static void GetDefaultBrowserUserEntries(
+  static void GetXPStyleDefaultBrowserUserEntries(
       BrowserDistribution* dist,
       const string16& chrome_exe,
       const string16& suffix,
@@ -482,8 +483,8 @@ class RegistryEntry {
     string16 chrome_open = ShellUtil::GetChromeShellOpenCmd(chrome_exe);
     string16 chrome_icon = ShellUtil::GetChromeIcon(dist, chrome_exe);
     for (int i = 0; ShellUtil::kBrowserProtocolAssociations[i] != NULL; i++) {
-      GetUserProtocolEntries(ShellUtil::kBrowserProtocolAssociations[i],
-                             chrome_icon, chrome_open, entries);
+      GetXPStyleUserProtocolEntries(ShellUtil::kBrowserProtocolAssociations[i],
+                                    chrome_icon, chrome_open, entries);
     }
 
     // start->Internet shortcut.
@@ -882,10 +883,10 @@ bool GetInstallationSpecificSuffix(BrowserDistribution*
dist,
   return ShellUtil::GetUserSpecificRegistrySuffix(suffix);
 }
 
-// Returns the root registry key (HKLM or HKCU) into which shell integration
-// registration for default protocols must be placed. As of Windows 8
everything
-// can go in HKCU for per-user installs.
-HKEY DetermineShellIntegrationRoot(bool is_per_user) {
+// Returns the root registry key (HKLM or HKCU) under which registrations must
+// be placed for this install. As of Windows 8 everything can go in HKCU for
+// per-user installs.
+HKEY DetermineRegistrationRoot(bool is_per_user) {
   return is_per_user && base::win::GetVersion() >= base::win::VERSION_WIN8 ?
       HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE;
 }
@@ -893,12 +894,12 @@ HKEY DetermineShellIntegrationRoot(bool is_per_user) {
 // Associates 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+ as well.
-bool RegisterChromeAsDefaultForXP(BrowserDistribution* dist,
-                                  int shell_change,
-                                  const string16& chrome_exe) {
+bool RegisterChromeAsDefaultXPStyle(BrowserDistribution* dist,
+                                    int shell_change,
+                                    const string16& chrome_exe) {
   bool ret = true;
   ScopedVector<RegistryEntry> entries;
-  RegistryEntry::GetDefaultBrowserUserEntries(
+  RegistryEntry::GetXPStyleDefaultBrowserUserEntries(
       dist, chrome_exe,
       ShellUtil::GetCurrentInstallationSuffix(dist, chrome_exe), &entries);
 
@@ -923,14 +924,14 @@ bool RegisterChromeAsDefaultForXP(BrowserDistribution*
dist,
 // required on Vista+ but since some applications still read these registry
 // keys directly, we have to do this on Vista+ as well.
 // See http://msdn.microsoft.com/library/aa767914.aspx for more details.
-bool RegisterChromeAsDefaultProtocolClientForXP(BrowserDistribution* dist,
-                                                const string16& chrome_exe,
-                                                const string16& protocol) {
+bool RegisterChromeAsDefaultProtocolClientXPStyle(BrowserDistribution* dist,
+                                                  const string16& chrome_exe,
+                                                  const string16& protocol) {
   ScopedVector<RegistryEntry> entries;
   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);
+  RegistryEntry::GetXPStyleUserProtocolEntries(protocol, chrome_icon,
+                                               chrome_open, &entries);
   // Change the default protocol handler for current user.
   if (!AddRegistryEntries(HKEY_CURRENT_USER, entries)) {
     LOG(ERROR) << "Could not make Chrome default protocol client (XP).";
@@ -1573,7 +1574,7 @@ bool ShellUtil::MakeChromeDefault(BrowserDistribution*
dist,
     }
   }
 
-  if (!RegisterChromeAsDefaultForXP(dist, shell_change, chrome_exe))
+  if (!RegisterChromeAsDefaultXPStyle(dist, shell_change, chrome_exe))
     ret = false;
 
   // Send Windows notification event so that it can update icons for
@@ -1601,7 +1602,7 @@ bool
ShellUtil::ShowMakeChromeDefaultSystemUI(BrowserDistribution* dist,
   const bool ret = LaunchSelectDefaultProtocolHandlerDialog(L"http");
 
   if (ret && IsChromeDefault())
-    RegisterChromeAsDefaultForXP(dist, CURRENT_USER, chrome_exe);
+    RegisterChromeAsDefaultXPStyle(dist, CURRENT_USER, chrome_exe);
 
   return ret;
 }
@@ -1643,9 +1644,9 @@ bool
ShellUtil::MakeChromeDefaultProtocolClient(BrowserDistribution* dist,
   }
 
   // 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.
-  if (!RegisterChromeAsDefaultProtocolClientForXP(dist, chrome_exe, protocol))
+  // should not be required on Vista+, but since some applications still read
+  // Software\Classes\<protocol> key directly, do this on Vista+ also.
+  if (!RegisterChromeAsDefaultProtocolClientXPStyle(dist, chrome_exe,
protocol))
     ret = false;
 
   return ret;
@@ -1672,7 +1673,7 @@ bool
ShellUtil::ShowMakeChromeDefaultProtocolClientSystemUI(
   const bool ret = LaunchSelectDefaultProtocolHandlerDialog(protocol.c_str());
 
   if (ret && IsChromeDefaultProtocolClient(protocol))
-    RegisterChromeAsDefaultProtocolClientForXP(dist, chrome_exe, protocol);
+    RegisterChromeAsDefaultProtocolClientXPStyle(dist, chrome_exe, protocol);
 
   return ret;
 }
@@ -1699,7 +1700,7 @@ bool ShellUtil::RegisterChromeBrowser(BrowserDistribution*
dist,
     return true;
 
   bool user_level = InstallUtil::IsPerUserInstall(chrome_exe.c_str());
-  HKEY root = DetermineShellIntegrationRoot(user_level);
+  HKEY root = DetermineRegistrationRoot(user_level);
 
   // Do the full registration if we can do it at user-level or if the user is
an
   // admin.
@@ -1771,7 +1772,7 @@ bool
ShellUtil::RegisterChromeForProtocol(BrowserDistribution* dist,
   if (IsChromeRegisteredForProtocol(dist, suffix, protocol))
     return true;
 
-  HKEY root = DetermineShellIntegrationRoot(
+  HKEY root = DetermineRegistrationRoot(
       InstallUtil::IsPerUserInstall(chrome_exe.c_str()));
 
   if (root == HKEY_CURRENT_USER || IsUserAnAdmin()) {

[commit-bot: I haz the power, 2012-10-17 14:30:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11066047/25001

[commit-bot: I haz the power, 2012-10-17 15:07:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-17 15:56:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11066047/23003

[commit-bot: I haz the power, 2012-10-17 17:58:50]

Change committed as 162456