Adding filter callback to BatchShortcutAction().

chrome/installer/util/shell_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // This file defines functions that integrate Chrome in Windows shell. These
 // functions can be used by Chrome as well as Chrome installer. All of the
 // work is done by the local functions defined in anonymous namespace in
 // this class.
 
 #include "chrome/installer/util/shell_util.h"
@@ skipping 1168 matching lines @@
       ShellUtil::GetBrowserModelId(dist, level == ShellUtil::CURRENT_USER));
   if (!base::DirectoryExists(folder)) {
     VLOG(1) << "No start screen shortcuts.";
     return false;
   }
 
   *path = folder;
   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())) {
     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 = base::DeleteFile(shortcut_path, false);
   LOG_IF(ERROR, !ret) << "Failed to remove " << shortcut_path.value();
   return ret;
 }
 
 bool ShortcutOpUpdate(const base::win::ShortcutProperties& shortcut_properties,
                       const base::FilePath& shortcut_path) {
   bool ret = base::win::CreateOrUpdateShortcutLink(
       shortcut_path, shortcut_properties, base::win::SHORTCUT_UPDATE_EXISTING);
   LOG_IF(ERROR, !ret) << "Failed to update " << 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
-// 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)) {
     LOG(WARNING) << "Cannot find path at location " << location;
     return false;
   }
 
   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;
       }
     } else {
       LOG(ERROR) << "Cannot resolve shortcut at " << shortcut_path.value();
       success = false;
     }
   }
   return success;
 }
@@ skipping 735 matching lines @@
                                 const base::FilePath& target_exe) {
   if (!ShellUtil::ShortcutLocationIsSupported(location))
     return true;  // Vacuous success.
 
   switch (location) {
     case SHORTCUT_LOCATION_START_MENU:  // Falls through.
     case SHORTCUT_LOCATION_APP_SHORTCUTS:
       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);
   }
 }
 
 // static
 bool ShellUtil::UpdateShortcuts(
     ShellUtil::ShortcutLocation location,
     BrowserDistribution* dist,
     ShellChange level,
     const base::FilePath& target_exe,
     const ShellUtil::ShortcutProperties& properties) {
   if (!ShellUtil::ShortcutLocationIsSupported(location))
     return true;  // Vacuous success.
 
   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) {
   // Use a thread-safe cache for the user's suffix.
   static base::LazyInstance<UserSpecificRegistrySuffix>::Leaky suffix_instance =
       LAZY_INSTANCE_INITIALIZER;
   return suffix_instance.Get().GetSuffix(suffix);
 }
 
 bool ShellUtil::GetOldUserSpecificRegistrySuffix(string16* suffix) {
@@ skipping 49 matching lines @@
     // are any left...).
     if (free_bits >= 8 && next_byte_index < size) {
       free_bits -= 8;
       bit_stream += bytes[next_byte_index++] << free_bits;
     }
   }
 
   DCHECK_EQ(ret.length(), encoded_length);
   return ret;
 }