Refactoring and tests for the highly undertested file_util::CreateOrUpdateShortcutLink() method.

Refactoring and tests for the highly undertested file_util::CreateOrUpdateShortcutLink() method.

Simplify file_util::CreateOrUpdateShortcutLink()'s interface (use a struct to set parameters passed which allows callers to specify exactly what they want without having to pass in a bunch of NULLs for the unused parameters).
The same concept will be used for ShellUtil's shortcut functions in an upcoming CL.

Moved ShellUtil::VerifyChromeShortcut() to file_util::VerifyShortcut() and augmented it for every shortcut properties. This will also allow other shortcut creators (web apps, profiles, etc.) to have a broader test coverage on the shortcut they create (i.e. more testable properties available).
I will leave it up to the owners of these various projects to augment their tests, this CL keeps the previously tested behavior, not more, not less.

This is the 1st CL of a massive refactoring effort for shortcuts (http://goo.gl/Az889) in which ShellUtil's shortcut methods have to be refactored (http://codereview.chromium.org/10836247/ : soon to incorporate interface changes from this CL) which led me even lower to first refactor file_util's shortcut methods.

BUG=132825
TEST=base_unittests --gtest_filter=FileUtilShortcutTest*
installer_util_unitests --gtest_filter=ShellUtilTestWithDirAndDist*
unit_tests --gtest_filter=ProfileShortcutManagerTest*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155869

[gab, 2012-09-05 22:22:13]

Robert, please take a look, feel free to defer to grt if you're overwhelmed, but
I figured I'd put you as you've been involved in the comments on the ShellUtil
CL leading me to this one.

@hallielaine: Take a look at
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc, with this CL
you will be now be able to test ALL properties you set on shortcuts, added a
TODO in your name to make this happen (would have done it, but the variables
required are not directly accessible and I don't want to interfere with your
profile shortcut work anymore than I have to!).

Cheers,
Gab

[robertshield, 2012-09-06 02:37:34]

Some preliminary nits. Overall, looks pretty good. Will think more on this
tomorrow.

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h#newcode236
base/file_util.h:236: // Propoerties for shortcuts. Properties set will be
applied to the shortcut
nit: *Properties

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h#newcode313
base/file_util.h:313: uint32 bitfield;
bitfield is a bad name for a member. Consider 'options' or similar.

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h#newcode332
base/file_util.h:332: // If |operation| is SHORTCUT_UPDATE_EXISTING| and
|shortcut_path| does not
nit: SHORTCUT_UPDATE_EXISTING (no |)

http://codereview.chromium.org/10914109/diff/2001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:465: // strings pointed to by |link_properties_*|
above.
nit: "strings pointed to" -> "paths referenced by"

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:398: DCHECK(properties.description.size() < MAX_PATH);
Could these DCHECKs be moved to the setters on ShortcutProperties?

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:440: const bool has_app_id =
don't think adding const here and on has_dual_mode is needed

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:443: (properties.bitfield &
ShortcutProperties::PROPERTIES_DUAL_MODE) != 0;
how about:
bool has_dual_mode = (properties.bitfield &
ShortcutProperties::PROPERTIES_DUAL_MODE);

http://codereview.chromium.org/10914109/diff/2001/base/test/test_file_util_wi...
File base/test/test_file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/test/test_file_util_wi...
base/test/test_file_util_win.cc:303: if ((properties.bitfield &
ShortcutProperties::PROPERTIES_TARGET) != 0 &&
None of the != 0s are needed here, are you using them for style reasons?

[gab, 2012-09-06 04:20:02]

Most nits addressed, see comments below.

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h#newcode236
base/file_util.h:236: // Propoerties for shortcuts. Properties set will be
applied to the shortcut
On 2012/09/06 02:37:34, robertshield wrote:
> nit: *Properties

Done.

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h#newcode313
base/file_util.h:313: uint32 bitfield;
On 2012/09/06 02:37:34, robertshield wrote:
> bitfield is a bad name for a member. Consider 'options' or similar. 

Done.

http://codereview.chromium.org/10914109/diff/2001/base/file_util.h#newcode332
base/file_util.h:332: // If |operation| is SHORTCUT_UPDATE_EXISTING| and
|shortcut_path| does not
On 2012/09/06 02:37:34, robertshield wrote:
> nit: SHORTCUT_UPDATE_EXISTING (no |)

Whoopsy!

http://codereview.chromium.org/10914109/diff/2001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:465: // strings pointed to by |link_properties_*|
above.
On 2012/09/06 02:37:34, robertshield wrote:
> nit: "strings pointed to" -> "paths referenced by"

Oops, thanks for catching this, this is actually no longer necessary as I'm now
passing string16's by reference; I used to pass in the underlying .c_str() (i.e.
wchar_t* parameters) in which case if the FilePath went out of scope that memory
area was up for grabs early while the pointer to |target| was still trusted....
(this is actually why I switched towards string16+bitfield instead of relying on
pointers+NULL; much safer now imo).

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:398: DCHECK(properties.description.size() < MAX_PATH);
On 2012/09/06 02:37:34, robertshield wrote:
> Could these DCHECKs be moved to the setters on ShortcutProperties?

Sounds good to me, unless an argument is made that these setters are getting too
complex and have to be moved in the cc file (as the DCHECKs now imply including
base/logging.h in the header (which was already done for OS_POSIX so that's no
SO bad))... I would rather keep it a simple struct fully implemented in the
header.

Done.

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:440: const bool has_app_id =
On 2012/09/06 02:37:34, robertshield wrote:
> don't think adding const here and on has_dual_mode is needed

I like const here because it's one of those scenarios where the only reason not
to inline this is that the value is needed twice, but really the value of these
should never change once it has been determined; thus const helps the reader
assume from this point on that these variables are just the result of whether
they were in properties.options point final.

I'll remove the consts if you insist.

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:443: (properties.bitfield &
ShortcutProperties::PROPERTIES_DUAL_MODE) != 0;
On 2012/09/06 02:37:34, robertshield wrote:
> how about:
> bool has_dual_mode = (properties.bitfield &
> ShortcutProperties::PROPERTIES_DUAL_MODE);

That'd be an implicit cast from int to bool. Not having != 0 directly in the ifs
is fine imo (which I just did uniformly) because in C++ any int other than 0 is
true in an if, but are we fine with an implicit cast (i.e. this is different as
we assign it to a "bool" type variable rather than using it directly in the if
as an int).

http://codereview.chromium.org/10914109/diff/2001/base/test/test_file_util_wi...
File base/test/test_file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/test/test_file_util_wi...
base/test/test_file_util_win.cc:303: if ((properties.bitfield &
ShortcutProperties::PROPERTIES_TARGET) != 0 &&
On 2012/09/06 02:37:34, robertshield wrote:
> None of the != 0s are needed here, are you using them for style reasons?

Right, done in file_util_win.cc as well.

[robertshield, 2012-09-06 19:20:01]

lgtm w/nits

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:440: const bool has_app_id =
On 2012/09/06 04:20:02, gab wrote:
> On 2012/09/06 02:37:34, robertshield wrote:
> > don't think adding const here and on has_dual_mode is needed
> 
> I like const here because it's one of those scenarios where the only reason
not
> to inline this is that the value is needed twice, but really the value of
these
> should never change once it has been determined; thus const helps the reader
> assume from this point on that these variables are just the result of whether
> they were in properties.options point final.
> 
> I'll remove the consts if you insist.

I'll insist a bit. The additional cognitive load incurred by requiring the
reader to parse another pair of keywords exceeds the practical benefit here imo.

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:443: (properties.bitfield &
ShortcutProperties::PROPERTIES_DUAL_MODE) != 0;
On 2012/09/06 04:20:02, gab wrote:
> On 2012/09/06 02:37:34, robertshield wrote:
> > how about:
> > bool has_dual_mode = (properties.bitfield &
> > ShortcutProperties::PROPERTIES_DUAL_MODE);
> 
> That'd be an implicit cast from int to bool. Not having != 0 directly in the
ifs
> is fine imo (which I just did uniformly) because in C++ any int other than 0
is
> true in an if, but are we fine with an implicit cast (i.e. this is different
as
> we assign it to a "bool" type variable rather than using it directly in the if
> as an int).

I feel it makes the code more readable and I don't see any contradicting
directives in the style guide.

http://codereview.chromium.org/10914109/diff/13006/base/test/test_file_util_w...
File base/test/test_file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/13006/base/test/test_file_util_w...
base/test/test_file_util_win.cc:309: ((::GetShortPathName(
Could you extract the path comparison into a helper function? I find this if
statement hard to read.

[gab, 2012-09-07 04:58:05]

Done, running Win try bots again: for some reasons the tests are failing on the
bots to verify the icon, but they never fail for me locally (I even tried a
local Chromium build and that passes); added some logging to the path
verification function, hopefully that gives me a hint.

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:440: const bool has_app_id =
On 2012/09/06 19:20:01, robertshield wrote:
> On 2012/09/06 04:20:02, gab wrote:
> > On 2012/09/06 02:37:34, robertshield wrote:
> > > don't think adding const here and on has_dual_mode is needed
> > 
> > I like const here because it's one of those scenarios where the only reason
> not
> > to inline this is that the value is needed twice, but really the value of
> these
> > should never change once it has been determined; thus const helps the reader
> > assume from this point on that these variables are just the result of
whether
> > they were in properties.options point final.
> > 
> > I'll remove the consts if you insist.
> 
> I'll insist a bit. The additional cognitive load incurred by requiring the
> reader to parse another pair of keywords exceeds the practical benefit here
imo.

Done.

http://codereview.chromium.org/10914109/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:443: (properties.bitfield &
ShortcutProperties::PROPERTIES_DUAL_MODE) != 0;
On 2012/09/06 19:20:01, robertshield wrote:
> On 2012/09/06 04:20:02, gab wrote:
> > On 2012/09/06 02:37:34, robertshield wrote:
> > > how about:
> > > bool has_dual_mode = (properties.bitfield &
> > > ShortcutProperties::PROPERTIES_DUAL_MODE);
> > 
> > That'd be an implicit cast from int to bool. Not having != 0 directly in the
> ifs
> > is fine imo (which I just did uniformly) because in C++ any int other than 0
> is
> > true in an if, but are we fine with an implicit cast (i.e. this is different
> as
> > we assign it to a "bool" type variable rather than using it directly in the
if
> > as an int).
> 
> I feel it makes the code more readable and I don't see any contradicting
> directives in the style guide.

Tried, but I'm getting warnings->errors:

g:\src\component\src\base\file_util_win.cc(437) : error C2220: warning treated
as error - no 'object' file generated
g:\src\component\src\base\file_util_win.cc(437) : warning C4800: 'const uint32'
: forcing value to bool 'true' or 'false

If that works for you, I'll keep the "!= 0" which I find cleaner than the "!!"
alternative.

http://codereview.chromium.org/10914109/diff/13006/base/test/test_file_util_w...
File base/test/test_file_util_win.cc (right):

http://codereview.chromium.org/10914109/diff/13006/base/test/test_file_util_w...
base/test/test_file_util_win.cc:309: ((::GetShortPathName(
On 2012/09/06 19:20:01, robertshield wrote:
> Could you extract the path comparison into a helper function? I find this if
> statement hard to read.

Done.

[gab, 2012-09-07 15:04:38]

Better path matching, @robertshield: PTAL.

[robertshield, 2012-09-07 15:46:56]

lgtm++

[gab, 2012-09-07 15:56:07]

brettw: base/* and chrome/* (except chrome/installer) OWNER approval (I can get
a more specific owner for chrome/browser/(profiles|ui) if you prefer, but given
these are just side-effects I didn't bother by default).

Thanks,
Gab

[brettw, 2012-09-08 18:28:47]

I feel like the file_util addition is quite large for this file which is already
a dumping ground. I'd feel better if this was in a new file base/win/shortcut.h
(using base::win namespace) which will make things a lot cleaner.

http://codereview.chromium.org/10914109/diff/2009/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10914109/diff/2009/base/file_util.h#newcode295
base/file_util.h:295: string16 target;
Should some of these be FilePaths instead? On Windows this should basically be a
string16 so I don't think you'll have problems. Same with working dir and icon.

[gab, 2012-09-10 17:35:33]

@brettw: Done.
Patch set 7: Moved all shortcut methods to base/win/shortcut.h, made a new
base/test/test_shortcut_win.h instead of modifying base/test/test_file_util.h.

Patch set 8: string16->FilePath where appropriate.

Thanks,
Gab

http://codereview.chromium.org/10914109/diff/2009/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10914109/diff/2009/base/file_util.h#newcode295
base/file_util.h:295: string16 target;
On 2012/09/08 18:28:48, brettw wrote:
> Should some of these be FilePaths instead? On Windows this should basically be
a
> string16 so I don't think you'll have problems. Same with working dir and
icon.

Done.

[grt (UTC plus 2), 2012-09-10 17:46:59]

a teeny tiny nit

http://codereview.chromium.org/10914109/diff/5013/base/win/shortcut.h
File base/win/shortcut.h (right):

http://codereview.chromium.org/10914109/diff/5013/base/win/shortcut.h#newcode139
base/win/shortcut.h:139: }  // namespace Win
nit: Win -> win

[gab, 2012-09-10 17:55:38]

Good catch, done.

@brettw: back to you.

http://codereview.chromium.org/10914109/diff/5013/base/win/shortcut.h
File base/win/shortcut.h (right):

http://codereview.chromium.org/10914109/diff/5013/base/win/shortcut.h#newcode139
base/win/shortcut.h:139: }  // namespace Win
On 2012/09/10 17:46:59, grt wrote:
> nit: Win -> win

Good catch (also in another file), done in both places.

[brettw, 2012-09-10 19:09:41]

Sweet, LGTM rubberstamp, I didn't look at the logic.

[gab, 2012-09-10 19:20:29]

Thanks,

@asanka: net/ OWNER rubberstamp (only a small side-effect)
@sky: ui/ OWNER rubberstamp (only a small side-effect)

[gab, 2012-09-10 20:49:13]

[Actually adding OWNERs this time]

@asanka: net/ OWNER rubberstamp (only a small side-effect)
@sky: ui/ OWNER rubberstamp (only a small side-effect)

[sky, 2012-09-10 21:12:02]

LGTM

[agl, 2012-09-10 21:14:51]

LGTM for net/

[commit-bot: I haz the power, 2012-09-10 21:15:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10914109/2053

[commit-bot: I haz the power, 2012-09-10 23:41:30]

Change committed as 155869

[dgrogan, 2012-09-11 02:52:37]

I reverted 155869 in 155918.  installer_util_unittests has failed on XP Tests
(dbg)(1) since this landed with the unhelpful:

[Running on builder: "XP Tests (dbg)(1)"]

E:\b\build\slave\XP_Tests__dbg__1_\build\src\build\Debug\installer_util_unittests.exe
--gtest_print_time
--gtest_output=xml:gtest-results\installer_util_unittests\installer_util_unittests.xml
No data was available to update the JSON results
Unable to generate JSON from XML, using log output.program finished with exit
code -1073741515
elapsedTime=2.047000

[dgrogan, 2012-09-11 02:52:37]

I reverted 155869 in 155918.  installer_util_unittests has failed on XP Tests
(dbg)(1) since this landed with the unhelpful:

[Running on builder: "XP Tests (dbg)(1)"]

E:\b\build\slave\XP_Tests__dbg__1_\build\src\build\Debug\installer_util_unittests.exe
--gtest_print_time
--gtest_output=xml:gtest-results\installer_util_unittests\installer_util_unittests.xml
No data was available to update the JSON results
Unable to generate JSON from XML, using log output.program finished with exit
code -1073741515
elapsedTime=2.047000