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

chrome/installer/setup/uninstall.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 the methods useful for uninstalling Chrome.
 
 #include "chrome/installer/setup/uninstall.h"
 
 #include <windows.h>
 
@@ skipping 310 matching lines @@
     }
   }
 
   if (kill) {
     VLOG(1) << installer::kChromeFrameHelperExe << " hung.  Killing.";
     base::CleanupProcesses(installer::kChromeFrameHelperExe, base::TimeDelta(),
                            content::RESULT_CODE_HUNG, NULL);
   }
 }
 
-// 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) {
+// 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();
-  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);
 
   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.";
   }
 }
 
 // Deletes shortcuts at |install_level| from Start menu, Desktop,
 // Quick Launch, taskbar, and secondary tiles on the Start Screen (Win8+).
 // Only shortcuts pointing to |target_exe| will be removed.
 void DeleteShortcuts(const InstallerState& installer_state,
                      const Product& product,
                      const base::FilePath& target_exe) {
@@ skipping 793 matching lines @@
     // Delete the key that delegate_execute might make.
     if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
       InstallUtil::DeleteRegistryKey(HKEY_CURRENT_USER,
                                      chrome::kMetroRegistryPath);
     }
 
     auto_launch_util::DisableAllAutoStartFeatures(
         ASCIIToUTF16(chrome::kInitialProfile));
 
     // Self-destruct flow: removing user-level Chrome because system-level
     // Chrome exists.

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

Restructure this comment to only mention what is happening here (i.e.,
retargeting); the comment below might be sufficient in itself).

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

Done.

     if (cmd_line.HasSwitch(installer::switches::kSelfDestruct) &&
         !installer_state.system_install()) {
       const base::FilePath system_chrome_path(
           GetChromeInstallPath(true, browser_dist).
               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

[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.";
       }
     }
 
     DeleteShortcuts(installer_state, product, base::FilePath(chrome_exe));
 
   } else if (product.is_chrome_app_host()) {
     const base::FilePath app_host_exe(
         installer_state.target_path().Append(installer::kChromeAppHostExe));
@@ skipping 283 matching lines @@
     // deletion unconditionally. If they are not empty, the session manager
     // will not delete them on reboot.
     ScheduleParentAndGrandparentForDeletion(target_path);
   } else if (DeleteApplicationProductAndVendorDirectories(target_path) ==
              installer::DELETE_FAILED) {
     *uninstall_status = installer::UNINSTALL_FAILED;
   }
 }
 
 }  // namespace installer