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..a00d96b09ea07a6da10b21ca2ec2048562910c05 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,33 +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+) Returns the folder for app shortcuts, or empty if none found.
+base::FilePath GetAppShortcutsFolder(BrowserDistribution* dist,

[gab, 2013/04/29 19:06:36]

I feel it would be better to have GetAppShortcutsFolder() use the same style as
GetShortcutPath() (i.e. return bool and return the path as out-param).
This would avoid returning empty paths here on failure and would integrate more
cleanly in GetShortcutPath().

[huangs, 2013/04/30 03:04:16]

Done.

+                                     bool is_user_level) {
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
+    return base::FilePath();
+
+  base::FilePath folder;
+  if (!PathService::Get(base::DIR_APP_SHORTCUTS, &folder)) {
+    LOG(ERROR) << "Could not get application shortcuts location.";
+    return base::FilePath();
+  }
+
+  folder = folder.Append(ShellUtil::GetBrowserModelId(dist, is_user_level));
+  if (!file_util::DirectoryExists(folder)) {
+    VLOG(1) << "No start screen shortcuts.";
+    return base::FilePath();
+  }
+
+  return folder;
+}
+
+const wchar_t kAllFiles[] = L"*";

[gab, 2013/04/29 19:06:36]

Rename kAllShortcuts and add the extension (if you can help it via a constant I
think it's better if the shortcut code doesn't have to fix this by adding the
extension).

[huangs, 2013/04/30 03:04:16]

Done.

+
+typedef base::Callback<bool(const base::FilePath&)> FileOperationCallback;
+
+// Shortcut operations for BatchShortcutAction().
+
+bool ShortcutOpNone(const base::FilePath& shortcut_path) {
+  return true;
+}
+
+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 FolderOpNone(const base::FilePath& folder_path) {
+  return true;
+}
+
+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 that have names matching
+// |name_filter|, and have |target_exe| as the target.

[gab, 2013/04/29 19:06:36]

Remove comma (it is only an enumeration of 2 things).

[huangs, 2013/04/30 03:04:16]

Rephrased.

+// Applies |opShortcut| on each matching shortcuts, then at the end, applies
+// |opFolder| on |shortcut_folder|.

[gab, 2013/04/29 19:06:36]

What about:

// Applies |shortcut_operation| on each matching shortcuts and then applies
|folder_operation| on |shortcut_folder|.

?

[huangs, 2013/04/30 03:04:16]

Done.

+// If |name_filter| is empty, then ignores |opShortcut|; otherwise iterate
+// with implicitly ".lnk" suffix. |kAllFiles| is used to specify all shortcuts.
+// Returns true iff all operations are successful.
+bool BatchShortcutAction(const FileOperationCallback& opShortcut,

[gab, 2013/04/29 19:06:36]

Use underscore casing for variable names, not Camel case.

I suggest shortcut_operation and folder_operation.

[huangs, 2013/04/30 03:04:16]

Done (language mix-up :).

+                         const FileOperationCallback& opFolder,
+                         ShellUtil::ShortcutLocation location,
+                         BrowserDistribution* dist,
+                         ShellUtil::ShellChange level,
+                         const base::FilePath& target_exe,
+                         const string16& name_filter) {
+  base::FilePath shortcut_folder;
+  if (!ShellUtil::GetShortcutPath(location, dist, level, &shortcut_folder) ||
+      !file_util::PathExists(shortcut_folder)) {
+    LOG(ERROR) << "Cannot find path at location " << location;

[gab, 2013/04/29 19:06:36]

This is more an WARNING, then an ERROR imo.

[huangs, 2013/04/30 03:04:16]

Done.

     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).";
+  // Operate on specified shortcuts in the folder.
+  InstallUtil::ProgramCompare target_compare(target_exe);
+  bool success = true;
+  if (!name_filter.empty()) {
+    // Ensure ".lnk" extension.
+    string16 pattern = name_filter + installer::kLnkExt;
+    if (!EndsWith(pattern, installer::kLnkExt, false))

[gab, 2013/04/29 19:06:36]

You just added installer::kLnkExt above so it will always end with the
extension...
I think you want:
string16 pattern = name_filter;
and then test for the extension and add it if necessary.

[huangs, 2013/04/30 03:04:16]

Ah, good catch!  Done.

+      pattern.append(installer::kLnkExt);
+    file_util::FileEnumerator enumerator(shortcut_folder, false,
+        file_util::FileEnumerator::FILES, pattern);

[gab, 2013/04/29 19:06:36]

either indent parameters at the paranthesis or 4 spaces in, not both.

[huangs, 2013/04/30 03:04:16]

Done.

+    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)) {
+          if (!opShortcut.Run(shortcut_path)) {

[gab, 2013/04/29 19:06:36]

Merge this if with the previous one via "&&" (since the previous if itself
doesn't do anything else but nest this one).

[huangs, 2013/04/30 03:04:16]

Done.

+            LOG(ERROR) << "Failed shortcut operation on "
+                       << shortcut_path.value();
+            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);
   }
 
-  // The shortcut at |shortcut_path| doesn't point to |target_exe|, act as if
-  // our shortcut had been deleted.
-  return true;
+  // Operate on shortcut folder itself.
+  if (!opFolder.Run(shortcut_folder)) {
+    LOG(ERROR) << "Failed folder operation on " << shortcut_folder.value();
+    success = false;
+  }
+  return success;
 }
 
 }  // namespace
@@ -1270,6 +1371,15 @@ bool ShellUtil::GetShortcutPath(ShellUtil::ShortcutLocation location,
                                           base::DIR_COMMON_START_MENU;
       add_folder_for_dist = true;
       break;
+    case SHORTCUT_LOCATION_TASKBAR_PINS:

[gab, 2013/04/29 19:06:36]

I feel this should fail if (base::win::GetVersion() >= base::win::VERSION_WIN7))
to be consistent with the new comments in the ShortcutLocation interface...

At the same time this probably doesn't matter since we should only ever use this
location to read/replace shortcut (all creations of pinned shortcuts are done
via the ShellAPI).

It also should really be up to PathService::Get() to fail for
base::DIR_TASKBAR_PINS under Win7, but it doesn't right now
(https://code.google.com/p/chromium/codesearch#chromium/src/base/base_paths_wi....
something to keep in mind/fix as a separate CL.

[huangs, 2013/04/30 03:04:16]

You mean, fail if (base::win::GetVersion() < base::win::VERSION_WIN7)?

Regarding PathService::Get(), I'll make a separate CL to fix this -- but not in
this CL, which is blocking another task.

+      dir_key = base::DIR_TASKBAR_PINS;
+      break;
+    case SHORTCUT_LOCATION_APP_SHORTCUTS: {
+      base::FilePath ret(GetAppShortcutsFolder(dist, level == CURRENT_USER));
+      if (!ret.empty())
+        *path = ret;
+      return !ret.empty();
+    }
     default:
       NOTREACHED();
       return false;
@@ -1299,8 +1409,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()) {

[gab, 2013/04/29 19:06:36]

GetShortcutPath() is already designed to only return true if the path is not
empty(), remove all these extra checks you added.

[huangs, 2013/04/30 03:04:16]

This IS the original code; my CL already removes these checks.

+  if (!GetShortcutPath(location, dist, SYSTEM_LEVEL, &system_shortcut_path)) {
     NOTREACHED();
     return false;
   }
@@ -1318,8 +1427,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 +1966,102 @@ bool ShellUtil::RegisterChromeForProtocol(BrowserDistribution* dist,
   }
 }
 
+// static
+bool ShellUtil::RemoveShortcutWithName(ShellUtil::ShortcutLocation location,
+                                       BrowserDistribution* dist,
+                                       ShellChange level,
+                                       const base::FilePath& target_path,
+                                       const string16& name_filter) {
+  bool delete_folder = (location == SHORTCUT_LOCATION_START_MENU);
+  return BatchShortcutAction(
+      base::Bind(&ShortcutOpUnpinAndDelete),
+      base::Bind(delete_folder ? &FolderOpDelete : &FolderOpNone),
+      location, dist, level, target_path, name_filter);
+}
+
+// static
 bool ShellUtil::RemoveShortcut(ShellUtil::ShortcutLocation location,

[gab, 2013/04/29 19:06:36]

Consider renaming to RemoveShortcutsAtLocation now that this no longer removes a
single shortcut.

[huangs, 2013/04/30 03:04:16]

Moot; removed routine.

                                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_path) {
+  return ShellUtil::RemoveShortcutWithName(
+      location, dist, level, target_path, kAllFiles);
 }
 
+// static
 void ShellUtil::RemoveTaskbarShortcuts(const string16& target_exe) {

[gab, 2013/04/29 19:06:36]

This call and RemoveStartScreenShortcuts() below do not have to be special cased
imo, they can easily be folded in the generic call above, no?

The early return by OS version will be handled by the path not being found on
older OS versions anyways.

(same comment for Migrate*)

[huangs, 2013/04/30 03:04:16]

Changed routine and the caller. To maintain behaviour, just making this case
unpin (and the caller in uninstlal.cc will need to know that this is for
user-level only).

   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"));
-
-  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());
-    }
-  }
+  ShortcutLocation location = SHORTCUT_LOCATION_TASKBAR_PINS;
+  ShellChange level = ShellUtil::CURRENT_USER;
+  BatchShortcutAction(base::Bind(&ShortcutOpUnpin), base::Bind(&FolderOpNone),
+                      location, NULL, level, base::FilePath(target_exe),
+                      kAllFiles);
 }
 
+// static
 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;
+  ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS;
+  ShellChange level = InstallUtil::IsPerUserInstall(target_exe.c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Removing start screen shortcuts.";
+  if (!BatchShortcutAction(base::Bind(&ShortcutOpNone),

[gab, 2013/04/29 19:06:36]

This will still unnecessarily iterate over all shortcuts. A Callback doesn't
have to be bound, in which case it is "null" (see [1] for example; I think here
you could use FileOperationCallback() directly). I think null callbacks are
preferable to empty operations here, you can then check that the callback isn't
null before starting to iterate.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_unit...

This is also true in the generic call if |delete_folder| is true since you
delete all the shortcuts one by one to later unconditionally delete the folder
recursively (making the individual shortcut operation superfluous).

[huangs, 2013/04/30 03:04:16]

This won't do useless cycles because |name_filter| was set to L"".

Defined kNullFileOperation and updated BatchShortcutAction() to skip if
|*_operation|.is_null().

+                           base::Bind(&FolderOpDelete),
+                           location, dist, level, base::FilePath(target_exe),
+                           L"")) {
+    LOG(ERROR) << "Failed to remove start screen shortcuts.";
   }
+}
+
+// static
+bool ShellUtil::MigrateShortcut(ShellUtil::ShortcutLocation location,
+                                BrowserDistribution* dist,
+                                ShellChange level,
+                                const base::FilePath& old_target_path,
+                                const base::FilePath& new_target_path) {

[gab, 2013/04/29 19:06:36]

A per above comment, change _path to _exe when referring to an executable rather
than a directory, easier to read when variable naming conventions are uniform.

[huangs, 2013/04/30 03:04:16]

Done.

+  return BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_path),
+                             base::Bind(&FolderOpNone),
+                             location, dist, level, old_target_path, kAllFiles);
+}
 
-  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.";
+// static
+void ShellUtil::MigrateTaskbarShortcuts(const string16& old_target_exe,
+                                        const string16& new_target_exe) {
+  if (base::win::GetVersion() < base::win::VERSION_WIN7)
     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();
+  base::FilePath new_target_path(new_target_exe);
+  ShortcutLocation location = SHORTCUT_LOCATION_TASKBAR_PINS;
+  ShellChange level = ShellUtil::CURRENT_USER;
+  BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_path),
+                      base::Bind(&FolderOpNone),
+                      location, NULL, level, base::FilePath(old_target_exe),
+                      kAllFiles);
+}
+
+// static
+void ShellUtil::MigrateStartScreenShortcuts(BrowserDistribution* dist,
+                                            const string16& old_target_exe,
+                                            const string16& new_target_exe) {
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
+    return;
+
+  base::FilePath new_target_path(new_target_exe);
+  ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS;
+  ShellChange level = InstallUtil::IsPerUserInstall(old_target_exe.c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Migrating start screen shortcuts.";
+  if (!BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_path),
+                           base::Bind(&FolderOpNone), location, dist, level,
+                           base::FilePath(old_target_exe), kAllFiles)) {
+    LOG(ERROR) << "Failed to migrate start screen shortcuts.";
   }
+
 }
 
 bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {