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..bc5de50d076c35847c7e57eb2a214e5004524dff 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,114 @@ 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 and stores an app shortcuts folder path in *|path|.
+// 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;
+}
+
+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;
+}
 
-  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 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|.
+// Applies |shortcut_operation| to each shortcut in |shortcut_folder| that
+// targets |target_exe|.
+// Returns true if all operations are successful. All intended operations are
+// performed even if failures occur.

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

s/performed/attempted ? (as there is no guarantee that they will actually be
performed/succeed).

[huangs, 2013/05/01 04:11:51]

Ah, of course. Done.

+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;
+  }
+
+  bool success = true;
+  InstallUtil::ProgramCompare target_compare(target_exe);
+  file_util::FileEnumerator enumerator(
+      shortcut_folder, false, file_util::FileEnumerator::FILES,
+      string16(L"*") + installer::kLnkExt);

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

Is string16() even required here?

[huangs, 2013/05/01 04:11:51]

Yes, because we cannot add two wchar_t*'s.

+  base::FilePath target_path;
+  for (base::FilePath shortcut_path = enumerator.Next();
+       !shortcut_path.empty();
+       shortcut_path = enumerator.Next()) {
+    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;
+    return false;
+  }
+  if (!file_util::Delete(shortcut_folder, true)) {
+    LOG(ERROR) << "Cannot remove folder " << shortcut_folder.value();
+    return false;
+  }
   return true;
 }
 
@@ -1254,6 +1337,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 +1354,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)
+        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 +1392,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 +1410,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,104 +1949,31 @@ 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);
-}
-
-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.";
-    return;
-  }
-
-  file_util::FileEnumerator shortcuts_enum(
-      taskbar_pins_path, false,
-      file_util::FileEnumerator::FILES, FILE_PATH_LITERAL("*.lnk"));
+// static
+bool ShellUtil::RemoveShortcuts(ShellUtil::ShortcutLocation location,
+                                BrowserDistribution* dist,
+                                ShellChange level,
+                                const base::FilePath& target_exe) {
+  if (location == SHORTCUT_LOCATION_START_MENU ||
+      location == SHORTCUT_LOCATION_APP_SHORTCUTS)
+    return RemoveShortcutFolder(location, dist, level);

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

nit: wrap in {} since condition spans more than one line.

[huangs, 2013/05/01 04:11:51]

Moot; using "switch" now.

 
-  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());
-    }
+  if (location == SHORTCUT_LOCATION_TASKBAR_PINS) {

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

Sweet, much cleaner :).

Use a switch(location) {} block instead of ifs though.

[huangs, 2013/05/01 04:11:51]

Done.

+    return BatchShortcutAction(base::Bind(&ShortcutOpUnpin), location, dist,
+                               level, target_exe);
   }
+  return BatchShortcutAction(base::Bind(&ShortcutOpDelete), location, dist,
+                             level, target_exe);
 }
 
-void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist,
-                                           const string16& 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();
-  }
+// 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);
 }
 
 bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {