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..3c7dbd987304df3ed63913d2df963601990e98e9 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,35 +1160,133 @@ 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|.
+// Returns true on success.
+bool GetAppShortcutsFolder(BrowserDistribution* dist,
+                                     bool is_user_level,

[grt (UTC plus 2), 2013/04/30 14:38:07]

indentation

[grt (UTC plus 2), 2013/04/30 14:38:07]

bool -> ShellChange for consistency

[huangs, 2013/04/30 15:58:38]

Done.

[huangs, 2013/04/30 15:58:38]

Done.

+                                     base::FilePath *path) {
+  DCHECK(path);
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
     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).";
-    }
-    if (delete_folder)
-      return file_util::Delete(shortcut_path.DirName(), true);
-    else
-      return file_util::Delete(shortcut_path, false);
+  base::FilePath folder;
+  if (!PathService::Get(base::DIR_APP_SHORTCUTS, &folder)) {
+    LOG(ERROR) << "Could not get application shortcuts location.";
+    return false;
+  }
+
+  folder = folder.Append(ShellUtil::GetBrowserModelId(dist, is_user_level));
+  if (!file_util::DirectoryExists(folder)) {
+    VLOG(1) << "No start screen shortcuts.";
+    return false;
   }
 
-  // The shortcut at |shortcut_path| doesn't point to |target_exe|, act as if
-  // our shortcut had been deleted.
+  *path = folder;
   return true;
 }
 
+const wchar_t kAllShortcuts[] = L"*.lnk";
+
+typedef base::Callback<bool(const base::FilePath&)> FileOperationCallback;
+
+FileOperationCallback kNullFileOperation;

[grt (UTC plus 2), 2013/04/30 14:38:07]

remove this (globals are banned).

[huangs, 2013/04/30 15:58:38]

Done. Replacing uses with 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) {
+  return file_util::Delete(shortcut_path, false);
+}
+
+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());
+  return base::win::CreateOrUpdateShortcutLink(
+      shortcut_path, shortcut_properties, base::win::SHORTCUT_REPLACE_EXISTING);
+}
+
+// Folder operations for BatchShortcutAction().
+
+bool FolderOpDelete(const base::FilePath& folder_path) {
+  return file_util::Delete(folder_path, true);
+}
+
+// {|location|, |dist|, |level|} determine |shortcut_folder| to operate in.
+// In |shortcut_folder|, iterate over shortcuts with names matching
+// |name_filter| and targets pointing to |target_exe|.
+// Applies |shortcut_operation| on each matching shortcuts and then applies
+// |folder_operation| on |shortcut_folder|.
+// If |name_filter| is empty, then ignores |shortcut_operation|; otherwise
+// iterate with implicitly ".lnk" suffix. |kAllShortcuts| is used to specify
+// all shortcuts.
+// Returns true iff all operations are successful.
+bool BatchShortcutAction(const FileOperationCallback& shortcut_operation,
+                         const FileOperationCallback& folder_operation,

[grt (UTC plus 2), 2013/04/30 14:38:07]

folder_operation seems overly abstract. it's only either null or delete, so how
about a delete_folder boolean? since it does a recursive delete, is there any
point in operating on any shortcuts in the folder since they will just be
deleted anyway?

[huangs, 2013/04/30 15:58:38]

Decided to extract RemoveShortcutFolder(). Before it was munged here to reuse
shortcut_folder. But the GetShortcutPath() cleanup makes it less expensive to
duplicate code to get |shortcut_folder|.

+                         ShellUtil::ShortcutLocation location,
+                         BrowserDistribution* dist,
+                         ShellUtil::ShellChange level,
+                         const base::FilePath& target_exe,
+                         const string16& name_filter) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

simplify by removing name_filter and kAllShortcuts and always use * + kLnkExt as
the enumerator pattern.

[huangs, 2013/04/30 15:58:38]

Done.

+  base::FilePath shortcut_folder;
+  if (!ShellUtil::GetShortcutPath(location, dist, level, &shortcut_folder) ||
+      !file_util::PathExists(shortcut_folder)) {

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

Why did you remove this PathExists() check? This is why I had suggested making
it a WARNING (i.e. the folder may simply not exist). GetShortcutPath() itself
should never fail.

The Create methods you might have taken this from are different in that they do
not need to verify that the folder exists because they will create it if it
doesn't (or silently fail in the case of an update).

In your case you are reading/deleting something and should make sure it exists
first imo (unless you can rely on FileEnumerator to successfully generate an
empty list if |shortcut_folder| doesn't exist?).

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

+    LOG(WARNING) << "Cannot find path at location " << location;
+    return false;
+  }
+
+  // Operate on specified shortcuts in the folder.
+  InstallUtil::ProgramCompare target_compare(target_exe);
+  bool success = true;
+  if (!shortcut_operation.is_null() && !name_filter.empty()) {
+    // Ensure ".lnk" extension.
+    string16 pattern = name_filter;
+    if (!EndsWith (pattern, installer::kLnkExt, false))

[grt (UTC plus 2), 2013/04/30 14:38:07]

remove space before open paren

[huangs, 2013/04/30 15:58:38]

Moot; deleted code.

+      pattern.append(installer::kLnkExt);
+    file_util::FileEnumerator enumerator(
+        shortcut_folder, false, file_util::FileEnumerator::FILES, pattern);
+    for (base::FilePath shortcut_path = enumerator.Next();
+         !shortcut_path.empty();
+         shortcut_path = enumerator.Next()) {
+      base::FilePath target_path;
+      if (base::win::ResolveShortcut(shortcut_path, &target_path, NULL)) {
+        if (target_compare.EvaluatePath(target_path) &&
+            !shortcut_operation.Run(shortcut_path)) {
+          LOG(ERROR) << "Failed shortcut operation on "

[grt (UTC plus 2), 2013/04/30 14:38:07]

suggestion: move the logging into the operations so that it can be more
meaningful.

[huangs, 2013/04/30 15:58:38]

Done.

+                     << shortcut_path.value();
+          success = false;
+        }
+      } else {
+        LOG(ERROR) << "Cannot resolve shortcut at " << shortcut_path.value();
+        success = false;
+      }
+    }
+  }
+
+  // Operate on shortcut folder itself.
+  if (!folder_operation.is_null() && !folder_operation.Run(shortcut_folder)) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

document that folder_operation (or delete_folder boolean if you follow my advice
above) takes place regardless of whether shortcut_operation succeeds or fails.
(and be sure that this is the right thing to do!)

[huangs, 2013/04/30 15:58:38]

Moved to RemoveShortcutFolder(). Right thing to do or not, it was the previous
thing -- and I'm just refactoring!

Added comment on BatchShortcutAction() to say that operations are performed even
if failures occur.

+    LOG(ERROR) << "Failed folder operation on " << shortcut_folder.value();
+    success = false;
+  }
+  return success;
+}
+
 }  // namespace
 
 const wchar_t* ShellUtil::kRegDefaultIcon = L"\\DefaultIcon";
@@ -1254,6 +1353,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 +1370,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 == CURRENT_USER, path);
+
     default:
       NOTREACHED();
       return false;
@@ -1299,8 +1408,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()) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

why remove this? it seems like a reasonable safeguard.

[huangs, 2013/04/30 15:58:38]

GetShortcutPath() already checks for this.

Removed instances in BatchShortcutAction() as well -- but will put it back if
you insist!

+  if (!GetShortcutPath(location, dist, SYSTEM_LEVEL, &system_shortcut_path)) {
     NOTREACHED();
     return false;
   }
@@ -1318,8 +1426,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 +1965,68 @@ bool ShellUtil::RegisterChromeForProtocol(BrowserDistribution* dist,
   }
 }
 
+// static
 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);
+                               const base::FilePath& target_exe) {
+  bool delete_file = (location != SHORTCUT_LOCATION_TASKBAR_PINS);
+  bool delete_folder = (location == SHORTCUT_LOCATION_START_MENU);
+  return BatchShortcutAction(
+      base::Bind(delete_file ? &ShortcutOpUnpinAndDelete : &ShortcutOpUnpin),
+      delete_folder ? base::Bind(&FolderOpDelete) : kNullFileOperation,
+      location, dist, level, target_exe, kAllShortcuts);
 }
 
-void ShellUtil::RemoveTaskbarShortcuts(const string16& target_exe) {
-  if (base::win::GetVersion() < base::win::VERSION_WIN7)
+// static
+void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist,
+                                           const base::FilePath& target_exe) {
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
     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;
+  ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS;

[grt (UTC plus 2), 2013/04/30 14:38:07]

nit: remove this local var and put the constant inline in the function call
below.

[huangs, 2013/04/30 15:58:38]

Done.

+  ShellChange level =
+      InstallUtil::IsPerUserInstall(target_exe.value().c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Removing start screen shortcuts.";
+  if (!BatchShortcutAction(kNullFileOperation,
+                           base::Bind(&FolderOpDelete),
+                           location, dist, level, base::FilePath(target_exe),

[grt (UTC plus 2), 2013/04/30 14:38:07]

base::FilePath(target_exe) -> target_exe

[huangs, 2013/04/30 15:58:38]

Ah, right. But moot: Removed code.

+                           L"")) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

L"" -> string16()

[huangs, 2013/04/30 15:58:38]

Moot; removed code.

+    LOG(ERROR) << "Failed to remove start screen shortcuts.";
   }
+}
 
-  file_util::FileEnumerator shortcuts_enum(
-      taskbar_pins_path, false,
-      file_util::FileEnumerator::FILES, FILE_PATH_LITERAL("*.lnk"));
-
-  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::MigrateShortcut(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),
+                             kNullFileOperation, location, dist, level,
+                             old_target_exe, kAllShortcuts);
 }
 
-void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist,
-                                           const string16& target_exe) {
+// static
+void ShellUtil::MigrateStartScreenShortcuts(
+    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;
+  ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS;
+  ShellChange level =
+      InstallUtil::IsPerUserInstall(old_target_exe.value().c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Migrating start screen shortcuts.";
+  if (!BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_exe),
+                           kNullFileOperation, location, dist, level,
+                           old_target_exe, kAllShortcuts)) {
+    LOG(ERROR) << "Failed to migrate start screen shortcuts.";
   }
 
-  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();
-  }
 }
 
 bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {