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..b34372e1980dc6a6feffd5d0483deb5a9bd19c96 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -328,52 +328,54 @@ 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) {
-  BrowserDistribution* dist = product.distribution();
+// Updates shortcuts to |old_target_exe| that have non-empty args, making them
+// target |new_target_exe| instead. This can only be called from user-level.

[gab, 2013/09/11 20:34:23]

s/from user-level/for user-level installs

[huangs, 2013/09/11 21:31:24]

Done.

+void RetargetUserShortcuts(const InstallerState& installer_state,
+                           const Product& product,
+                           const base::FilePath& old_target_exe,
+                           const base::FilePath& new_target_exe) {
+  if (installer_state.system_install()) {
+    NOTREACHED();
+    return;
+  }
   DCHECK(!installer_state.system_install());

[gab, 2013/09/11 20:34:23]

Remove this line (same as above if).

[huangs, 2013/09/11 21:31:24]

Oops. Done.

+  BrowserDistribution* dist = product.distribution();
   ShellUtil::ShellChange install_level = ShellUtil::CURRENT_USER;
   ShellUtil::ShortcutProperties updated_properties(install_level);
   updated_properties.set_target(new_target_exe);
 
   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(

[gab, 2013/09/11 20:34:23]

Can we at least add a TODO to put these in a for loop after calamity@'s CL? What
blocks us from doing it without his CL? And which CL are we talking about
exactly -- he did land a few recently?

[huangs, 2013/09/11 21:31:24]

I'm thinking of https://chromiumcodereview.appspot.com/13864015/ , which should
add the "Chrome Apps" folder.

Added TODO(huangs).

+          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.";
   }
 }
@@ -1199,8 +1201,8 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
         // system-level chrome.exe. Heuristic: consider only shortcuts that have
         // non-empty args. Therefore the main user-level chrome.exe will not get

[gab, 2013/09/11 20:34:23]

Such detail about the heuristic, etc. belong in the implementation not the call
site.

[huangs, 2013/09/11 21:31:24]

Done.

         // retarged, and will get deleted by DeleteShortcuts() below.
-        RetargetShortcuts(installer_state, product, base::FilePath(chrome_exe),
-                          system_chrome_path, true);
+        RetargetUserShortcuts(installer_state, product,
+                              base::FilePath(chrome_exe), system_chrome_path);
       } else {
         VLOG(1) << "Retarget failed: system-level Chrome not found.";
       }