Adding filter callback to BatchShortcutAction().

Adding filter callback to BatchShortcutAction().

We wish to enable allow better selectivity in ShellUtil::RemoveShortcuts() and ShellUtil::UpdateShortcuts() to decide on which shortcuts to operate on. In this CL:
- BatchShortcutAction() is given a (newly defined) FileFilterCallback predicate that decides whether or not a given shortcut should be operated on.
- Currently this is only created by (newly defined) class FilterTargetEq.
- Also renamed FileOperationCallback to ShortcutOperationCallback.
- This is a refactoring CL, so we retain the old behavior: BatchShortcutAction() only considers shortcuts that have with a specific target.

BUG=235857
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217092

[huangs, 2013-08-08 19:32:44]

PTAL. This is just a refactoring CL.

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

Comments below, please also update the CL description to match the new patch
set.

Cheers,
Gab

https://codereview.chromium.org/22607006/diff/1/chrome/installer/util/shell_u...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/22607006/diff/1/chrome/installer/util/shell_u...
chrome/installer/util/shell_util.cc:1192: FileFilterCallback;
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.

https://codereview.chromium.org/22607006/diff/1/chrome/installer/util/shell_u...
chrome/installer/util/shell_util.cc:1198: explicit FilterTargetEq(const
base::FilePath& filt_target_exe);
Abbreviations are frowned upon in chrome.

Use something like |desired_target_exe| instead?

Same comment about filt_target_compare_ below.

https://codereview.chromium.org/22607006/diff/1/chrome/installer/util/shell_u...
chrome/installer/util/shell_util.cc:1200: // Returns true if |target_path|
matches the arget stored.
s/arget/target

https://codereview.chromium.org/22607006/diff/1/chrome/installer/util/shell_u...
chrome/installer/util/shell_util.cc:1204: FileFilterCallback Bind();
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...
).

https://codereview.chromium.org/22607006/diff/1/chrome/installer/util/shell_u...
chrome/installer/util/shell_util.cc:1224: typedef base::Callback<bool(const
base::FilePath&)> FileOperationCallback;
This one is fine as a general "File operation" I guess, but what about also
renaming this to ShortcutOperationCallback for consistency with comment above?

https://codereview.chromium.org/22607006/diff/1/chrome/installer/util/shell_u...
chrome/installer/util/shell_util.cc:2039: base::Bind(&ShortcutOpDelete),
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]

PTAL.

https://chromiumcodereview.appspot.com/22607006/diff/1/chrome/installer/util/...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/22607006/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1192: FileFilterCallback;
Done. Also added variable names in side /* comments */; same below.

https://chromiumcodereview.appspot.com/22607006/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1198: explicit FilterTargetEq(const
base::FilePath& filt_target_exe);
Done.

https://chromiumcodereview.appspot.com/22607006/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1200: // Returns true if |target_path|
matches the arget stored.
On 2013/08/09 13:40:47, gab wrote:
> s/arget/target

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

https://chromiumcodereview.appspot.com/22607006/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1204: FileFilterCallback Bind();
Also copied comments regarding lifetime.

https://chromiumcodereview.appspot.com/22607006/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:1224: typedef base::Callback<bool(const
base::FilePath&)> FileOperationCallback;
On 2013/08/09 13:40:47, gab wrote:
> This one is fine as a general "File operation" I guess, but what about also
> renaming this to ShortcutOperationCallback for consistency with comment above?

Done.

https://chromiumcodereview.appspot.com/22607006/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.cc:2039: base::Bind(&ShortcutOpDelete),
Done.  :)

[gab, 2013-08-09 15:10:17]

lgtm w/ nit

Thanks!
Gab

https://codereview.chromium.org/22607006/diff/6001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/22607006/diff/6001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:2037: .AsShortcutFilterCallback(),
nit: convention is to leave the '.' on the previous line to make it obvious the
statement in not over.
(change here and below)

optional: Consider changing this to wrapping the parameters 4 spaces in on the
next line.

[huangs, 2013-08-09 15:27:23]

Thanks! Going through CQ.

https://chromiumcodereview.appspot.com/22607006/diff/6001/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/22607006/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:2037: .AsShortcutFilterCallback(),
Fixed ".", but not wrapping parameter, because for my upcoming change there will
be another parameter (and expanding this into 3 lines would be odd).

[commit-bot: I haz the power, 2013-08-09 15:28:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/22607006/11001

[commit-bot: I haz the power, 2013-08-09 22:51:57]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 02:02:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/22607006/11001

[commit-bot: I haz the power, 2013-08-10 06:27:12]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-12 13:32:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/22607006/42001

[commit-bot: I haz the power, 2013-08-12 22:02:32]

Change committed as 217092