Adding filter callback to BatchShortcutAction().

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 9ef0876b7bc3faa512191b8715584e2bc774ec8a..3fc32e51b4461d3e50df3e861d1ce9fdd8ea6fb4 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -1186,10 +1186,43 @@ bool GetAppShortcutsFolder(BrowserDistribution* dist,
   return true;
 }
 
-typedef base::Callback<bool(const base::FilePath&)> FileOperationCallback;
+// Shortcut filters for BatchShortcutAction().
+
+typedef base::Callback<bool(const base::FilePath&, const string16&)>
+    FileFilterCallback;

[gab, 2013/08/09 13:40:47]

I don't think it makes sense to make these generic "FileFilterCallback"s that
take a magic string16.

A more explicit ShortcutFilterCallback is better I'd say since that's all it's
used for and the extra string16 parameter is obscur otherwise.

[huangs, 2013/08/09 15:06:20]

Done. Also added variable names in side /* comments */; same below.

+
+// FilterTargetEq is a shortcut filter that matches only shortcuts that have a
+// specific target.
+class FilterTargetEq {
+ public:
+  explicit FilterTargetEq(const base::FilePath& filt_target_exe);

[gab, 2013/08/09 13:40:47]

Abbreviations are frowned upon in chrome.

Use something like |desired_target_exe| instead?

Same comment about filt_target_compare_ below.

[huangs, 2013/08/09 15:06:20]

Done.

+
+  // Returns true if |target_path| matches the arget stored.

[gab, 2013/08/09 13:40:47]

s/arget/target

[huangs, 2013/08/09 15:06:20]

Done; elaborated a bit more (this will be extended in the next CL).

+  bool Match(const base::FilePath& target_path, const string16& args) const;
+
+  // A convenience routine that creates a callback to call Match().
+  FileFilterCallback Bind();

[gab, 2013/08/09 13:40:47]

Rename this method to AsShortcutFilterCallback(), such is the norm around the
codebase for classes returning special callbacks to themselves (e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
).

[huangs, 2013/08/09 15:06:20]

Also copied comments regarding lifetime.

+
+ private:
+  InstallUtil::ProgramCompare filt_target_compare_;
+};
+
+FilterTargetEq::FilterTargetEq(const base::FilePath& filt_target_exe)
+    : filt_target_compare_(filt_target_exe) {}
+
+bool FilterTargetEq::Match(const base::FilePath& target_path,
+                           const string16& args) const {
+  return filt_target_compare_.EvaluatePath(target_path);
+}
+
+FileFilterCallback FilterTargetEq::Bind() {
+  return base::Bind(&FilterTargetEq::Match, base::Unretained(this));
+}
 
 // Shortcut operations for BatchShortcutAction().
 
+typedef base::Callback<bool(const base::FilePath&)> FileOperationCallback;

[gab, 2013/08/09 13:40:47]

This one is fine as a general "File operation" I guess, but what about also
renaming this to ShortcutOperationCallback for consistency with comment above?

[huangs, 2013/08/09 15:06:20]

Done.

+
 bool ShortcutOpUnpin(const base::FilePath& shortcut_path) {
   VLOG(1) << "Trying to unpin " << shortcut_path.value();
   if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) {
@@ -1214,15 +1247,14 @@ bool ShortcutOpUpdate(const base::win::ShortcutProperties& shortcut_properties,
 }
 
 // {|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
-// attempted even if failures occur.
-bool BatchShortcutAction(const FileOperationCallback& shortcut_operation,
+// For each shortcut in |shortcut_folder| that match |shortcut_filter|, apply
+// |shortcut_operation|. Returns true if all operations are successful.
+// All intended operations are attempted, even if failures occur.
+bool BatchShortcutAction(const FileFilterCallback& shortcut_filter,
+                         const FileOperationCallback& shortcut_operation,
                          ShellUtil::ShortcutLocation location,
                          BrowserDistribution* dist,
-                         ShellUtil::ShellChange level,
-                         const base::FilePath& target_exe) {
+                         ShellUtil::ShellChange level) {
   DCHECK(!shortcut_operation.is_null());
   base::FilePath shortcut_folder;
   if (!ShellUtil::GetShortcutPath(location, dist, level, &shortcut_folder)) {
@@ -1231,16 +1263,16 @@ bool BatchShortcutAction(const FileOperationCallback& shortcut_operation,
   }
 
   bool success = true;
-  InstallUtil::ProgramCompare target_compare(target_exe);
   base::FileEnumerator enumerator(
       shortcut_folder, false, base::FileEnumerator::FILES,
       string16(L"*") + installer::kLnkExt);
   base::FilePath target_path;
+  string16 args;
   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) &&
+    if (base::win::ResolveShortcut(shortcut_path, &target_path, &args)) {
+      if (shortcut_filter.Run(target_path, args) &&
           !shortcut_operation.Run(shortcut_path)) {
         success = false;
       }
@@ -1996,12 +2028,18 @@ bool ShellUtil::RemoveShortcuts(ShellUtil::ShortcutLocation location,
       return RemoveShortcutFolder(location, dist, level);
 
     case SHORTCUT_LOCATION_TASKBAR_PINS:
-      return BatchShortcutAction(base::Bind(&ShortcutOpUnpin), location, dist,
-                                 level, target_exe);
+      return BatchShortcutAction(FilterTargetEq(target_exe).Bind(),
+                                 base::Bind(&ShortcutOpUnpin),
+                                 location,
+                                 dist,
+                                 level);
 
     default:
-      return BatchShortcutAction(base::Bind(&ShortcutOpDelete), location, dist,
-                                 level, target_exe);
+      return BatchShortcutAction(FilterTargetEq(target_exe).Bind(),
+                                 base::Bind(&ShortcutOpDelete),

[gab, 2013/08/09 13:40:47]

I overall like this refactoring very much since it is now explicit at the call
site that this will apply the ShortcutOpDelete action on shortcuts for which
FilterTargetEq(target_exe) matches.

[huangs, 2013/08/09 15:06:20]

Done.  :)

+                                 location,
+                                 dist,
+                                 level);
   }
 }
 
@@ -2017,8 +2055,11 @@ bool ShellUtil::UpdateShortcuts(
 
   base::win::ShortcutProperties shortcut_properties(
       TranslateShortcutProperties(properties));
-  return BatchShortcutAction(base::Bind(&ShortcutOpUpdate, shortcut_properties),
-                             location, dist, level, target_exe);
+  return BatchShortcutAction(FilterTargetEq(target_exe).Bind(),
+                             base::Bind(&ShortcutOpUpdate, shortcut_properties),
+                             location,
+                             dist,
+                             level);
 }
 
 bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {