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..c09f7b6970b05c1c70e74ac106fbf3aa694fbb34 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -1159,32 +1159,212 @@ 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,
+                                     const string16& target_exe) {
+  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,

[gab, 2013/04/24 21:41:46]

nit: wrap |dist| on the line below.

The rule is either you wrap to the paran or 4 spaces, but never a mix of both.

[huangs, 2013/04/25 16:27:46]

Done.

+          InstallUtil::IsPerUserInstall(target_exe.c_str())));
+  if (!file_util::DirectoryExists(folder)) {
+    VLOG(1) << "No start screen shortcuts.";
+    return base::FilePath();
+  }
+
+  return folder;
+}
+
+const wchar_t kAllFiles[] = L"*";
+
+// Helper to perform common operations to shortcuts in a folder.
+// Implement the ActOnShortcut() and ActOnFolder() to define operations.
+class BatchShortcutOperation {

[gab, 2013/04/24 21:41:46]

I think it would be nicer, less verbose, and easier to specialize if instead of
a full class..........

[huangs, 2013/04/25 16:27:46]

I think it's important to group it into a class, but using a different method
now.

+ public:
+  BatchShortcutOperation() {}
+  virtual ~BatchShortcutOperation() {}
+
+  // Goes through |shortcut_folder| and seeks shortcuts with names that match
+  // |name_filter| (empty = none), and with targets given by |target_exe|.
+  // Calls ActOnShortcut() with each matching shortcuts. Afterwards, calls
+  // ActOnFolder() with |shortcut_folder|.
+  // Returns true iff all operations are successful.
+  bool Run(const base::FilePath& shortcut_folder,
+           const string16& name_filter,
+           const base::FilePath& target_exe);
+
+  // Wrapper for Run(): first determine shortcut path based on given data.
+  // |name| can specify the name for a single shortcut (without ".lnk" suffix),

[gab, 2013/04/24 21:41:46]

s/suffix/extension

[gab, 2013/04/24 21:41:46]

I don't like relying on the extension being present or not, often adds burden on
the callers either way. How about simply checking for it and adjusting if
required (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u...).

I could be convinced otherwise..

[huangs, 2013/04/25 16:27:46]

Done.

[huangs, 2013/04/25 16:27:46]

Done; adding ".lnk" if missing (exception is empty string, which specifies "only
act on folder").

+  // or all shortcuts if NULL.
+  bool RunForLocation(ShellUtil::ShortcutLocation location,
+                      BrowserDistribution* dist,
+                      const base::FilePath& target_exe,
+                      ShellUtil::ShellChange level,
+                      const string16* name);
+
+  // Wrapper for Run(): specifying all shortcuts in the taskbar.
+  bool RunForTaskbar(const base::FilePath& target_exe);
+
+  // Returns: true on success.
+  virtual bool ActOnShortcut(const base::FilePath& shortcut_path) const = 0;
+
+  // Returns: true on success.
+  virtual bool ActOnFolder(const base::FilePath& shortcut_folder) const = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BatchShortcutOperation);
+};
+
+// |name_filter| will have ".lnk" appended.
+bool BatchShortcutOperation::Run(const base::FilePath& shortcut_folder,
+                                 const string16& name_filter,
+                                 const base::FilePath& target_exe) {
+  InstallUtil::ProgramCompare target_compare(target_exe);
+  bool success = true;
+  if (!name_filter.empty()) {
+    string16 pattern = name_filter + 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()) {
+      if (shortcut_path.Extension() == installer::kLnkExt) {

[gab, 2013/04/24 21:41:46]

You are already only enumerating files with the .lnk extension.

[huangs, 2013/04/25 16:27:46]

Changed to DCHECK_EQ().

+        base::FilePath target_path;
+        if (base::win::ResolveShortcut(shortcut_path, &target_path, NULL)) {
+          if (target_compare.EvaluatePath(target_path))
+            success = ActOnShortcut(shortcut_path) && success;
+        } else {
+          LOG(ERROR) << "Cannot resolve shortcut at " << shortcut_path.value();
+          success = false;
+        }
+      }
+    }
+  }
+  success = ActOnFolder(shortcut_folder) && success;
+  return success;
+}
+
+bool BatchShortcutOperation::RunForLocation(
+    ShellUtil::ShortcutLocation location,
+    BrowserDistribution* dist,
+    const base::FilePath& target_exe,
+    ShellUtil::ShellChange level,
+    const string16* name) {

[gab, 2013/04/24 21:41:46]

If |name| is changed to a pattern (as suggested in another comment in
shell_util.h), I don't think we need 2 methods (i.e., I think we can fold Run()
in this method).

[huangs, 2013/04/25 16:27:46]

I think I'll get rid of the ActOnFolder() stuff; will get back to you.

+  base::FilePath shortcut_folder;
+  if (!ShellUtil::GetShortcutPath(location, dist, level, &shortcut_folder) ||
+      shortcut_folder.empty()) {
+    NOTREACHED();
     return false;
+  }
 
-  if (InstallUtil::ProgramCompare(target_exe).EvaluatePath(target_path)) {
-    // Unpin the shortcut if it was ever pinned by the user or the installer.
+  return Run(shortcut_folder, name ? *name : kAllFiles, target_exe);
+}
+
+bool BatchShortcutOperation::RunForTaskbar(const base::FilePath& target_exe) {
+  base::FilePath taskbar_pins_path;
+  if (!PathService::Get(base::DIR_TASKBAR_PINS, &taskbar_pins_path) ||

[gab, 2013/04/24 21:41:46]

Feels like we should augment GetShortcutPath() instead of adding an extra method
here.

[huangs, 2013/04/25 16:27:46]

Might be solved by removing ActOnFolder().

+      !file_util::PathExists(taskbar_pins_path)) {
+    LOG(ERROR) << "Couldn't find path to taskbar pins.";
+    return false;
+  }
+
+  return Run(taskbar_pins_path, kAllFiles, target_exe);

[gab, 2013/04/24 21:41:46]

Taskbar pins are special though, they have to be explicitly unpinned via
base::win::TaskbarUnpinShortcutLink(), only deleting the file confuses Windows.

base::win::TaskbarUnpinShortcutLink() can either be called on the shortcut that
was originally pinned or on the pin itself (in fact I think it resolves the
appid of the "unpinned shortcut" and unpins the shortcut with a matching appid).

[huangs, 2013/04/25 16:27:46]

Per earlier discussion, having a dedicated unpin operation would probably solve
this.

+}
+
+// Batch shortcut operation to unpin and/or delete shortcuts in a folder,
+// and possibly remove the folder itself.
+class BatchShortcutDelete: public BatchShortcutOperation {
+ public:
+  BatchShortcutDelete(bool unpin, bool delete_file, bool delete_folder);
+  virtual ~BatchShortcutDelete() {}
+
+  // Unpins and/or removes |shortcut_path|.
+  // Returns true if deletion successful or if no deletion occurs.
+  virtual bool ActOnShortcut(const base::FilePath& shortcut_path) const
+      OVERRIDE;
+
+  // Removes |shortcut_folder| if |delete_folder_|.
+  // Returns true if deletion successful.
+  virtual bool ActOnFolder(const base::FilePath& shortcut_folder) const
+      OVERRIDE;
+
+ private:
+  bool unpin_;
+  bool delete_file_;
+  bool delete_folder_;
+
+  DISALLOW_COPY_AND_ASSIGN(BatchShortcutDelete);
+};
+
+BatchShortcutDelete::BatchShortcutDelete(bool unpin,
+                                         bool delete_file,
+                                         bool delete_folder)

[gab, 2013/04/24 21:41:46]

(bool, bool, bool, bool, bool, bool, bool) interfaces are hard to read :).

Consider using a struct so that it is easier to see what is being called from
the caller code without having to check the method's signature.

[huangs, 2013/04/25 16:27:46]

Adding accessors to explicitly set flags.  But this may change once I remove
ActOnFolder().

+  : unpin_(unpin),
+    delete_file_(delete_file),
+    delete_folder_(delete_folder) {
+}
+
+bool BatchShortcutDelete::ActOnShortcut(const base::FilePath& shortcut_path)
+    const {
+  if (unpin_) {
     VLOG(1) << "Trying to unpin " << shortcut_path.value();
     if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) {

[gab, 2013/04/24 21:41:46]

Did you check who uses RemoveShortcut(), is it still worth it for this method to
perform the unpinning or should that be done separately?

[huangs, 2013/04/25 16:27:46]

There a routine that avoids calling this BECAUSE of unpinning.  But this should
be a separate refactoring task!

       VLOG(1) << shortcut_path.value()
               << " wasn't pinned (or the unpin failed).";
+      // Not an error condition, since shortcut might not be pinned.
     }
-    if (delete_folder)
-      return file_util::Delete(shortcut_path.DirName(), true);
-    else
-      return file_util::Delete(shortcut_path, false);
   }
+  return delete_file_ ? file_util::Delete(shortcut_path, false) : true;
+}
+
+bool BatchShortcutDelete::ActOnFolder(const base::FilePath& shortcut_folder)
+    const {
+  return delete_folder_ ? file_util::Delete(shortcut_folder, true) : true;
+}
+
+// Batch shortcut operation to set target.
+class BatchShortcutSetTarget: public BatchShortcutOperation {
+ public:
+  explicit BatchShortcutSetTarget(const base::FilePath& target);
+  virtual ~BatchShortcutSetTarget() {}
+
+  // Sets the target of |shortcut_path| to |target_|.
+  // Returns true on success.
+  virtual bool ActOnShortcut(const base::FilePath& shortcut_path) const
+      OVERRIDE;
+
+  // No-op; returns true.
+  virtual bool ActOnFolder(const base::FilePath& shortcut_path) const OVERRIDE;
+
+ private:
+  base::FilePath target_;
+
+  DISALLOW_COPY_AND_ASSIGN(BatchShortcutSetTarget);
+};
+
+BatchShortcutSetTarget::BatchShortcutSetTarget(
+    const base::FilePath& target)
+  : target_(target) {
+}
+
+bool BatchShortcutSetTarget::ActOnShortcut(const base::FilePath& shortcut_path)
+    const {
+  base::win::ShortcutProperties shortcut_properties;
+  shortcut_properties.set_target(target_);
+  shortcut_properties.set_working_dir(target_.DirName());
+  return base::win::CreateOrUpdateShortcutLink(
+      shortcut_path, shortcut_properties, base::win::SHORTCUT_UPDATE_EXISTING);
+}
 
-  // The shortcut at |shortcut_path| doesn't point to |target_exe|, act as if
-  // our shortcut had been deleted.
+bool BatchShortcutSetTarget::ActOnFolder(const base::FilePath& shortcut_path)
+    const {
   return true;
 }
 
@@ -1858,103 +2038,85 @@ 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);
+  bool delete_folder = (location == SHORTCUT_LOCATION_START_MENU);
+  BatchShortcutDelete op(true, true, delete_folder);  // Unpon and delete-file.

[gab, 2013/04/24 21:41:46]

nit: Unpon :)

[huangs, 2013/04/25 16:27:46]

Done. :)

+  return op.RunForLocation(location, dist, target_exe, level, shortcut_name);
 }
 
+// static
 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"));
-
-  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());
-    }
-  }
+  BatchShortcutDelete op(true, false, false);  // Unpin only.
+  op.RunForTaskbar(base::FilePath(target_exe));
 }
 
+// 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.";
+  base::FilePath folder(GetAppShortcutsFolder(dist, target_exe));

[gab, 2013/04/24 21:41:46]

This could probably also go in GetShortcutPath() as an extra ShortcutLocation,
making the code even simpler.

[huangs, 2013/04/25 16:27:46]

Will do this next.  Can I remove / redo the messages below?

+  if (folder.empty())
     return;
+
+  BatchShortcutDelete op(false, false, true);  // Delete-folder only.
+  VLOG(1) << "Removing start screen shortcuts from " << folder.value();
+  if (!op.Run(folder, L"", base::FilePath())) {
+    LOG(ERROR) << "Failed to remove start screen shortcuts from "
+               << folder.value();
   }
+}
+
+// static
+bool ShellUtil::MigrateShortcut(ShellUtil::ShortcutLocation location,
+                                BrowserDistribution* dist,
+                                const base::FilePath& old_target_exe,
+                                const base::FilePath& new_target_exe,
+                                ShellChange level,
+                                const string16* shortcut_name) {
+  bool delete_folder = (location == SHORTCUT_LOCATION_START_MENU);
+  BatchShortcutSetTarget op(new_target_exe);
+  return op.RunForLocation(location, dist, old_target_exe, level,
+                           shortcut_name);
+}
 
-  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);
+  BatchShortcutSetTarget op(new_target_path);
+  op.RunForTaskbar(base::FilePath(old_target_exe));
+}
+
+// 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 folder(GetAppShortcutsFolder(dist, old_target_exe));
+  if (folder.empty())
+    return;
+
+  base::FilePath new_target_path(new_target_exe);
+  BatchShortcutSetTarget op(new_target_path);
+  VLOG(1) << "Migrating start screen shortcuts from " << folder.value();
+  if (!op.Run(folder, kAllFiles, base::FilePath(old_target_exe))) {
+    LOG(ERROR) << "Failed to migrate start screen shortcuts from "
+               << folder.value();
   }
 }