Restricting scope of ShellUtil's shortcut update feature for its specific usage in Chrome self-dest…

chrome/installer/setup/uninstall.cc

Index: chrome/installer/setup/uninstall.cc
diff --git a/chrome/installer/setup/uninstall.cc b/chrome/installer/setup/uninstall.cc
index 8693dfa834d46f353876cf4909977a942142f695..cf0caa907c8a75cfa31ded9b9ed1f14dadd651a3 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -328,52 +328,57 @@ void CloseChromeFrameHelperProcess() {
   }
 }
 
-// Updates shortcuts to |old_target_exe| to target |new_target_exe| instead. If
-// |require_args| is set, then only updates shortcuts with non-empty targets.
-// This should only be called from user-level.
-void RetargetShortcuts(const InstallerState& installer_state,
-                       const Product& product,
-                       const base::FilePath& old_target_exe,
-                       const base::FilePath& new_target_exe,
-                       bool require_args) {
+

[gab, 2013/09/13 01:53:26]

nit: Remove this extra empty line.

[huangs, 2013/09/13 04:16:26]

Done.

+// Updates shortcuts to |old_target_exe| that have non-empty args, making them
+// target |new_target_exe| instead. The non-empty args requirement is a
+// heuristic to determine whether a shortcut is "user-generated". This routine
+// can only be called for user-level installs.
+void RetargetUserShortcuts(const InstallerState& installer_state,

[gab, 2013/09/13 01:53:26]

WDYT of calling this method RetargetUserShortcutsWithArgs to be consistent with
the other method's name (since you are describing the empty args heuristic on
this method anyways and it will always be used that way).

[huangs, 2013/09/13 04:16:26]

I was sneaky and used the word "User" to refer to 2 things:
- this updates user-level shortcuts,
- this update "user-created shortcut", and the "With args" part is
implementation detail that the caller should not know or care.

But then you're using "owned non-default shortcut" below. So following your
suggested change.

+                           const Product& product,
+                           const base::FilePath& old_target_exe,
+                           const base::FilePath& new_target_exe) {
+  if (installer_state.system_install()) {
+    NOTREACHED();
+    return;
+  }
   BrowserDistribution* dist = product.distribution();
-  DCHECK(!installer_state.system_install());
   ShellUtil::ShellChange install_level = ShellUtil::CURRENT_USER;
   ShellUtil::ShortcutProperties updated_properties(install_level);
   updated_properties.set_target(new_target_exe);
 
+  // TODO(huangs): Make this data-driven, along with DeleteShortcuts().
   VLOG(1) << "Retargeting Desktop shortcuts.";
-  if (!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist,
-                                  install_level, old_target_exe, require_args,
-                                  updated_properties)) {
+  if (!ShellUtil::UpdateShortcutsWithArgs(
+          ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist, install_level,
+          old_target_exe, updated_properties)) {
     LOG(WARNING) << "Failed to retarget Desktop shortcuts.";
   }
 
   VLOG(1) << "Retargeting Quick Launch shortcuts.";
-  if (!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_QUICK_LAUNCH,
-                                  dist, install_level, old_target_exe,
-                                  require_args, updated_properties)) {
+  if (!ShellUtil::UpdateShortcutsWithArgs(
+          ShellUtil::SHORTCUT_LOCATION_QUICK_LAUNCH, dist, install_level,
+          old_target_exe, updated_properties)) {
     LOG(WARNING) << "Failed to retarget Quick Launch shortcuts.";
   }
 
   VLOG(1) << "Retargeting Start Menu shortcuts.";
-  if (!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_START_MENU, dist,
-                                  install_level, old_target_exe, require_args,
-                                  updated_properties)) {
+  if (!ShellUtil::UpdateShortcutsWithArgs(
+          ShellUtil::SHORTCUT_LOCATION_START_MENU, dist, install_level,
+          old_target_exe, updated_properties)) {
     LOG(WARNING) << "Failed to retarget Start Menu shortcuts.";
   }
 
   // Retarget pinned-to-taskbar shortcuts that point to |chrome_exe|.
-  if (!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_TASKBAR_PINS,
-                                  dist, ShellUtil::CURRENT_USER, old_target_exe,
-                                  require_args, updated_properties)) {
+  if (!ShellUtil::UpdateShortcutsWithArgs(
+          ShellUtil::SHORTCUT_LOCATION_TASKBAR_PINS, dist,
+          ShellUtil::CURRENT_USER, old_target_exe, updated_properties)) {
     LOG(WARNING) << "Failed to retarget taskbar shortcuts at user-level.";
   }
 
   // Retarget the folder of secondary tiles from the start screen for |dist|.
-  if (!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_APP_SHORTCUTS,
-                                  dist, install_level, old_target_exe,
-                                  require_args, updated_properties)) {
+  if (!ShellUtil::UpdateShortcutsWithArgs(
+          ShellUtil::SHORTCUT_LOCATION_APP_SHORTCUTS, dist, install_level,
+          old_target_exe, updated_properties)) {
     LOG(WARNING) << "Failed to retarget start-screen shortcuts.";
   }
 }
@@ -1187,7 +1192,8 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
         ASCIIToUTF16(chrome::kInitialProfile));
 
     // Self-destruct flow: removing user-level Chrome because system-level
-    // Chrome exists.
+    // Chrome exists. Also, make user-generated shortcuts retarget system-level
+    // Chrome.

[gab, 2013/09/13 01:53:26]

I still think the first part of this comment is overdescriptive, i.e. the only
thing this if does is the retargeting; what about:

// If user-level chrome is self-destructing as a result of encountering a
system-level chrome, retarget owned non-default shortcuts (app shortcuts,
profile shortcuts, etc.) to the system-level chrome.

?

[huangs, 2013/09/13 04:16:26]

Done. I take that your use of "chrome" (generic goodness) instead of "Chrome"
(the browser) is intentional, but is it appropriate?

[gab, 2013/09/13 14:31:24]

Yes it is intentional, except that it's chrome (the browser) instead of Chrome
(short name for Google Chrome).

     if (cmd_line.HasSwitch(installer::switches::kSelfDestruct) &&
         !installer_state.system_install()) {
       const base::FilePath system_chrome_path(
@@ -1195,12 +1201,11 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
               Append(installer::kChromeExe));
       VLOG(1) << "Retargeting user-generated Chrome shortcuts.";
       if (base::PathExists(system_chrome_path)) {
-        // Retarget all user-generated shortcuts to user-level chrome.exe to
-        // system-level chrome.exe. Heuristic: consider only shortcuts that have
-        // non-empty args. Therefore the main user-level chrome.exe will not get
-        // retarged, and will get deleted by DeleteShortcuts() below.
-        RetargetShortcuts(installer_state, product, base::FilePath(chrome_exe),
-                          system_chrome_path, true);
+        // Update all user-generated user-level chrome.exe shortcuts, so they
+        // retarget system-level chrome.exe. In particular, this excludes the
+        // main shortcuts to user-level chrome.exe, so it get deleted below.

[gab, 2013/09/13 01:53:26]

I'd say the suggested comment above is sufficient in itself; I don't mind
putting here or above, but I don't find have two comments is very useful here.

[huangs, 2013/09/13 04:16:26]

Removed comment.

+        RetargetUserShortcuts(installer_state, product,
+                              base::FilePath(chrome_exe), system_chrome_path);
       } else {
         VLOG(1) << "Retarget failed: system-level Chrome not found.";
       }