Refactoring installer shortcut deletion; adding dedicated shortcut update feature.

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 4a60171174d5526e3b21bab49799e9406b19f4c5..ad81e9660f60a7af86ab6008b64a13354880ef5d 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -15,6 +15,7 @@
 #include <limits>
 #include <string>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
@@ -1159,32 +1160,123 @@ ShellUtil::DefaultState ProbeProtocolHandlers(
   return ProbeOpenCommandHandlers(protocols, num_protocols);
 }
 
-// Removes shortcut at |shortcut_path| if it is a shortcut that points to
-// |target_exe|. If |delete_folder| is true, deletes the parent folder of
-// the shortcut completely. Returns true if either the shortcut was deleted
-// successfully or if the shortcut did not point to |target_exe|.
-bool MaybeRemoveShortcutAtPath(const base::FilePath& shortcut_path,
-                               const base::FilePath& target_exe,
-                               bool delete_folder) {
-  base::FilePath target_path;
-  if (!base::win::ResolveShortcut(shortcut_path, &target_path, NULL))
+// (Windows 8+) Finds the folder for app shortcuts, store it in *|path|.

[gab, 2013/04/30 19:20:33]

s/store/stores

[huangs, 2013/04/30 20:52:10]

Rephrased.

+// Returns true on success.
+bool GetAppShortcutsFolder(BrowserDistribution* dist,
+                           ShellUtil::ShellChange level,
+                           base::FilePath *path) {
+  DCHECK(path);
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
+    return false;
+
+  base::FilePath folder;
+  if (!PathService::Get(base::DIR_APP_SHORTCUTS, &folder) || folder.empty()) {
+    LOG(ERROR) << "Could not get application shortcuts location.";
+    return false;
+  }
+
+  folder = folder.Append(ShellUtil::GetBrowserModelId(
+                             dist, level == ShellUtil::CURRENT_USER));
+  if (!file_util::DirectoryExists(folder)) {
+    VLOG(1) << "No start screen shortcuts.";
+    return false;
+  }
+
+  *path = folder;
+  return true;
+}
+
+const wchar_t kAllShortcuts[] = L"*.lnk";
+
+typedef base::Callback<bool(const base::FilePath&)> FileOperationCallback;
+
+// Shortcut operations for BatchShortcutAction().
+
+bool ShortcutOpUnpin(const base::FilePath& shortcut_path) {
+  VLOG(1) << "Trying to unpin " << shortcut_path.value();
+  if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) {
+    VLOG(1) << shortcut_path.value() << " wasn't pinned (or the unpin failed).";
+    // No error, since shortcut might not be pinned.
+  }
+  return true;
+}
+
+bool ShortcutOpDelete(const base::FilePath& shortcut_path) {
+  bool ret = file_util::Delete(shortcut_path, false);
+  LOG_IF(ERROR, !ret) << "Failed to remove " << shortcut_path.value();
+  return ret;
+}
+
+bool ShortcutOpUnpinAndDelete(const base::FilePath& shortcut_path) {
+  // Want to try both, without short-circuiting.
+  bool ret1 = ShortcutOpUnpin(shortcut_path);
+  bool ret2 = ShortcutOpDelete(shortcut_path);
+  return ret1 && ret2;
+}
+
+bool ShortcutOpSetTarget(const base::FilePath& target_path,
+                         const base::FilePath& shortcut_path) {
+  base::win::ShortcutProperties shortcut_properties;
+  shortcut_properties.set_target(target_path);
+  shortcut_properties.set_working_dir(target_path.DirName());
+  bool ret = base::win::CreateOrUpdateShortcutLink(
+      shortcut_path, shortcut_properties, base::win::SHORTCUT_REPLACE_EXISTING);
+  LOG_IF(ERROR, !ret) << "Failed to retarget " << shortcut_path.value();
+  return ret;
+}
+
+// {|location|, |dist|, |level|} determine |shortcut_folder|.
+// In |shortcut_folder|, applies |shortcut_operation| to each shortcut that

[gab, 2013/04/30 19:20:33]

I find this wording weird, I'd prefer:

Applies |shortcut_operation| to each shortcut in |shortcut_folder| that points
to |target_exe| .

[huangs, 2013/04/30 20:52:10]

Much better.  Done.

+// point to |target_exe|,

[gab, 2013/04/30 19:20:33]

dot, not comma, at end of sentence.

[huangs, 2013/04/30 20:52:10]

Done.

+// Returns true if all operations are successful. All intended operations are
+// performed even if failures occur.
+bool BatchShortcutAction(const FileOperationCallback& shortcut_operation,
+                         ShellUtil::ShortcutLocation location,
+                         BrowserDistribution* dist,
+                         ShellUtil::ShellChange level,
+                         const base::FilePath& target_exe) {
+  DCHECK(!shortcut_operation.is_null());
+  base::FilePath shortcut_folder;
+  if (!ShellUtil::GetShortcutPath(location, dist, level, &shortcut_folder)) {
+    LOG(WARNING) << "Cannot find path at location " << location;
     return false;
+  }
 
-  if (InstallUtil::ProgramCompare(target_exe).EvaluatePath(target_path)) {
-    // Unpin the shortcut if it was ever pinned by the user or the installer.
-    VLOG(1) << "Trying to unpin " << shortcut_path.value();
-    if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) {
-      VLOG(1) << shortcut_path.value()
-              << " wasn't pinned (or the unpin failed).";
+  bool success = true;
+  InstallUtil::ProgramCompare target_compare(target_exe);
+  file_util::FileEnumerator enumerator(
+      shortcut_folder, false, file_util::FileEnumerator::FILES,
+      kAllShortcuts);

[gab, 2013/04/30 19:20:33]

I think it's fine to hardcode ("*" + installer::kLnkExt) here instead of using
the global constant.

[grt (UTC plus 2), 2013/04/30 19:59:53]

+1

[huangs, 2013/04/30 20:52:10]

Done.

[huangs, 2013/04/30 20:52:10]

Done.

+  for (base::FilePath shortcut_path = enumerator.Next();
+       !shortcut_path.empty();
+       shortcut_path = enumerator.Next()) {
+    base::FilePath target_path;

[grt (UTC plus 2), 2013/04/30 19:59:53]

nit: move this out of the loop so it isn't created/destroyed on each iteration

[huangs, 2013/04/30 20:52:10]

Done.

+    if (base::win::ResolveShortcut(shortcut_path, &target_path, NULL)) {
+      if (target_compare.EvaluatePath(target_path) &&
+          !shortcut_operation.Run(shortcut_path)) {
+        success = false;
+      }
+    } else {
+      LOG(ERROR) << "Cannot resolve shortcut at " << shortcut_path.value();
+      success = false;
     }
-    if (delete_folder)
-      return file_util::Delete(shortcut_path.DirName(), true);
-    else
-      return file_util::Delete(shortcut_path, false);
   }
+  return success;
+}
 
-  // The shortcut at |shortcut_path| doesn't point to |target_exe|, act as if
-  // our shortcut had been deleted.
+// Removes folder spsecified by {|location|, |dist|, |level|}.
+bool RemoveShortcutFolder(ShellUtil::ShortcutLocation location,
+                          BrowserDistribution* dist,
+                          ShellUtil::ShellChange level) {
+  base::FilePath shortcut_folder;
+  if (!ShellUtil::GetShortcutPath(location, dist, level, &shortcut_folder)) {
+    LOG(WARNING) << "Cannot find path at location " << location;

[gab, 2013/04/30 19:20:33]

See other other comment on "why did you remove the check for PathExists()?"

And same comment here, I guess it's OK if you can rely on file_util::Delete to
succeed when the path doesn't exist.

Either way GetShortcutPath() failing by itself is a NOTREACHED() case.

[grt (UTC plus 2), 2013/04/30 19:59:53]

Looks like this isn't always the case -- on Win8 false may legitimately be
returned for SHORTCUT_LOCATION_APP_SHORTCUTS if the dir isn't there.

[huangs, 2013/04/30 20:52:10]

The NOTREACHED() case is only for cases where folders are expected to exist;
this is not invoked for SHORTCUT_LOCATION_TASKBAR_PINS or
SHORTCUT_LOCATION_APP_SHORTCUTS.

Awaiting decisions on whether or not to put extra check.

[huangs, 2013/04/30 20:52:10]

Acknowledged.

[gab, 2013/04/30 22:30:22]

Ok so lets keep LOG(WARNING), but also check !PathExists()?

+    return false;
+  }
+  if (!file_util::Delete(shortcut_folder, true)) {
+    LOG(ERROR) << "Cannot remove folder " << shortcut_folder.value();
+    return false;
+  }
   return true;
 }
 
@@ -1254,6 +1346,7 @@ bool ShellUtil::GetShortcutPath(ShellUtil::ShortcutLocation location,
                                 BrowserDistribution* dist,
                                 ShellChange level,
                                 base::FilePath* path) {
+  DCHECK(path);
   int dir_key = -1;
   bool add_folder_for_dist = false;
   switch (location) {
@@ -1270,6 +1363,15 @@ bool ShellUtil::GetShortcutPath(ShellUtil::ShortcutLocation location,
                                           base::DIR_COMMON_START_MENU;
       add_folder_for_dist = true;
       break;
+    case SHORTCUT_LOCATION_TASKBAR_PINS:
+      if (base::win::GetVersion() < base::win::VERSION_WIN7)

[gab, 2013/04/30 19:20:33]

I don't believe this should take care of this logic, base_paths_win.cc should do
that for base::DIR_TASKBAR_PINS imo.

[huangs, 2013/04/30 20:52:10]

Modified in base_paths_win.cc.

There are many unneeded "base::", but this is not the time to change it.

[gab, 2013/04/30 22:30:22]

I didn't meant to change it in this CL, I think it's fine to fix this in a
closely following CL.

[grt (UTC plus 2), 2013/05/01 13:35:58]

Could you explain?

[huangs, 2013/05/01 15:56:29]

I was commenting on

https://chromiumcodereview.appspot.com/14287008/diff/56001/base/base_paths_wi...

Even though it's in namespace base, "base::" is still used everywhere.  All
their base is not belong to there. :)

+        return false;
+
+      dir_key = base::DIR_TASKBAR_PINS;
+      break;
+    case SHORTCUT_LOCATION_APP_SHORTCUTS:
+      return GetAppShortcutsFolder(dist, level, path);
+
     default:
       NOTREACHED();
       return false;
@@ -1299,8 +1401,7 @@ bool ShellUtil::CreateOrUpdateShortcut(
 
   base::FilePath user_shortcut_path;
   base::FilePath system_shortcut_path;
-  if (!GetShortcutPath(location, dist, SYSTEM_LEVEL, &system_shortcut_path) ||
-      system_shortcut_path.empty()) {
+  if (!GetShortcutPath(location, dist, SYSTEM_LEVEL, &system_shortcut_path)) {
     NOTREACHED();
     return false;
   }
@@ -1318,8 +1419,7 @@ bool ShellUtil::CreateOrUpdateShortcut(
     // Otherwise install the user-level shortcut, unless the system-level
     // variant of this shortcut is present on the machine and |operation| states
     // not to create a user-level shortcut in that case.
-    if (!GetShortcutPath(location, dist, CURRENT_USER, &user_shortcut_path) ||
-        user_shortcut_path.empty()) {
+    if (!GetShortcutPath(location, dist, CURRENT_USER, &user_shortcut_path)) {
       NOTREACHED();
       return false;
     }
@@ -1858,103 +1958,64 @@ bool ShellUtil::RegisterChromeForProtocol(BrowserDistribution* dist,
   }
 }
 
-bool ShellUtil::RemoveShortcut(ShellUtil::ShortcutLocation location,
-                               BrowserDistribution* dist,
-                               const base::FilePath& target_exe,
-                               ShellChange level,
-                               const string16* shortcut_name) {
-  const bool delete_folder = (location == SHORTCUT_LOCATION_START_MENU);
-
-  base::FilePath shortcut_folder;
-  if (!GetShortcutPath(location, dist, level, &shortcut_folder) ||
-      shortcut_folder.empty()) {
-    NOTREACHED();
-    return false;
-  }
-
-  if (!delete_folder && !shortcut_name) {
-    file_util::FileEnumerator enumerator(shortcut_folder, false,
-        file_util::FileEnumerator::FILES);
-    bool had_failures = false;
-    for (base::FilePath path = enumerator.Next(); !path.empty();
-         path = enumerator.Next()) {
-      if (path.Extension() != installer::kLnkExt)
-        continue;
-
-      if (!MaybeRemoveShortcutAtPath(path, target_exe, delete_folder))
-        had_failures = true;
-    }
-    return !had_failures;
-  }
-
-  const string16 shortcut_base_name(
-      (shortcut_name ? *shortcut_name : dist->GetAppShortCutName()) +
-      installer::kLnkExt);
-  const base::FilePath shortcut_path(
-      shortcut_folder.Append(shortcut_base_name));
-  if (!file_util::PathExists(shortcut_path))
-    return true;
-
-  return MaybeRemoveShortcutAtPath(shortcut_path, target_exe, delete_folder);
+// static
+bool ShellUtil::RemoveShortcuts(ShellUtil::ShortcutLocation location,
+                                BrowserDistribution* dist,
+                                ShellChange level,
+                                const base::FilePath& target_exe) {
+  const FileOperationCallback file_op =
+      (location == SHORTCUT_LOCATION_TASKBAR_PINS) ?
+      base::Bind(&ShortcutOpUnpin) : base::Bind(&ShortcutOpUnpinAndDelete);

[gab, 2013/04/30 19:20:33]

indent this line 4 more spaces in.

[huangs, 2013/04/30 20:52:10]

Done.

+  bool success = true;

[grt (UTC plus 2), 2013/04/30 19:59:53]

bool success = BatchShortcutAction(...);

[huangs, 2013/04/30 20:52:10]

Now returning directly, so got rid of variable |success|,

+  if (!BatchShortcutAction(file_op, location, dist, level, target_exe))
+    success = false;
+  if (location == SHORTCUT_LOCATION_START_MENU &&

[gab, 2013/04/30 19:20:33]

Put this first and only do the BatchShortcutAction if not doing the
RemoveShortcutFolder() -- it's useless to delete shortcuts one-by-one only to
delete the entire folder latter (given it's an unconditional recursive folder
deletion...).

If your main concern is the fact that this wouldn't call unpin for the shortcuts
in that folder, you are right, but I don't think this matters anymore now that
we do a cleanup pass for the pinned shortcuts, I'd suggest removing the
ShortcutOpUnpinAndDelete operation altogether, using ShortcutOpDelete instead
and relying on the BatchShortcutAction on SHORTCUT_LOCATION_TASKBAR_PINS to take
care of all the pins (this didn't use to exist hence the need to unpin
individual shortcuts, it was added to cleanup non-installer-pinned-shortcuts and
now that it cleans all pinned shortcuts, it's unnecessary for individual
shortcuts to unpin themselves imo!).

[huangs, 2013/04/30 20:52:10]

Done; doing 3 distinct things for 3 separate cases.

Also fixing caller comments in uninstall.cc.

+      !RemoveShortcutFolder(location, dist, level)) {
+    success = false;
+  }
+  return success;
 }
 
-void ShellUtil::RemoveTaskbarShortcuts(const string16& target_exe) {
-  if (base::win::GetVersion() < base::win::VERSION_WIN7)
-    return;
-
-  base::FilePath taskbar_pins_path;
-  if (!PathService::Get(base::DIR_TASKBAR_PINS, &taskbar_pins_path) ||
-      !file_util::PathExists(taskbar_pins_path)) {
-    LOG(ERROR) << "Couldn't find path to taskbar pins.";
+// static
+void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist,

[gab, 2013/04/30 19:20:33]

Once again, I think this can be merged in RemoveShortcuts, the logic for |level|
can be left to the caller as it already is for other locations and the
RemoveShortcutFolder call is then the same logic as the Start Menu shortcuts.

[huangs, 2013/04/30 20:52:10]

Done. Key Changes:
- Updated the single caller from uninstall.cc.
- Instead of grabbing level from taget_exe, it is assigned by the caller to be
|install_level|.
- The verbose message is gone (can put it in the caller if needed).

+                                           const base::FilePath& target_exe) {
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
     return;
-  }
 
-  file_util::FileEnumerator shortcuts_enum(
-      taskbar_pins_path, false,
-      file_util::FileEnumerator::FILES, FILE_PATH_LITERAL("*.lnk"));
+  ShellChange level =
+      InstallUtil::IsPerUserInstall(target_exe.value().c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Removing start screen shortcuts.";
+  if (!RemoveShortcutFolder(SHORTCUT_LOCATION_APP_SHORTCUTS, dist, level))
+    LOG(ERROR) << "Failed to remove start screen shortcuts.";
+}
 
-  base::FilePath target_path(target_exe);
-  InstallUtil::ProgramCompare target_compare(target_path);
-  for (base::FilePath shortcut_path = shortcuts_enum.Next();
-       !shortcut_path.empty();
-       shortcut_path = shortcuts_enum.Next()) {
-    base::FilePath read_target;
-    if (!base::win::ResolveShortcut(shortcut_path, &read_target, NULL)) {
-      LOG(ERROR) << "Couldn't resolve shortcut at " << shortcut_path.value();
-      continue;
-    }
-    if (target_compare.EvaluatePath(read_target)) {
-      // Unpin this shortcut if it points to |target_exe|.
-      base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str());
-    }
-  }
+// static
+bool ShellUtil::RetargetShortcuts(ShellUtil::ShortcutLocation location,
+                                  BrowserDistribution* dist,
+                                  ShellChange level,
+                                  const base::FilePath& old_target_exe,
+                                  const base::FilePath& new_target_exe) {
+  return BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_exe),
+                             location, dist, level, old_target_exe);
 }
 
-void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist,
-                                           const string16& target_exe) {
+// static
+void ShellUtil::RetargetStartScreenShortcuts(

[gab, 2013/04/30 19:20:33]

This can also easily be folded in RetargetShortcuts imo.

[huangs, 2013/04/30 20:52:10]

Done.

+    BrowserDistribution* dist,
+    const base::FilePath& old_target_exe,
+    const base::FilePath& new_target_exe) {
   if (base::win::GetVersion() < base::win::VERSION_WIN8)
     return;
 
-  base::FilePath app_shortcuts_path;
-  if (!PathService::Get(base::DIR_APP_SHORTCUTS, &app_shortcuts_path)) {
-    LOG(ERROR) << "Could not get application shortcuts location to delete"
-               << " start screen shortcuts.";
-    return;
-  }
-
-  app_shortcuts_path = app_shortcuts_path.Append(
-      GetBrowserModelId(dist,
-                        InstallUtil::IsPerUserInstall(target_exe.c_str())));
-  if (!file_util::DirectoryExists(app_shortcuts_path)) {
-    VLOG(1) << "No start screen shortcuts to delete.";
-    return;
-  }
-
-  VLOG(1) << "Removing start screen shortcuts from "
-          << app_shortcuts_path.value();
-  if (!file_util::Delete(app_shortcuts_path, true)) {
-    LOG(ERROR) << "Failed to remove start screen shortcuts from "
-               << app_shortcuts_path.value();
+  ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS;
+  ShellChange level =
+      InstallUtil::IsPerUserInstall(old_target_exe.value().c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Retargeting start screen shortcuts.";
+  if (!BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_exe),
+                           location, dist, level, old_target_exe)) {
+    LOG(ERROR) << "Failed to retarget start screen shortcuts.";
   }
 }