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) {
+

[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.";
   }
 }
 
 // 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.
+    // 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(
           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
-        // 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.";
       }
     }
 
     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