Refactor ShellUtil shortcut code -- single multi-purpose methods as opposed to many slighlty diffe…

Refactor ShellUtil shortcut code  -- single multi-purpose methods as opposed to many slighlty different methods.

Merge Quick Launch + Desktop logic into same call and add start menu shortcut logic in the mix too.

Make properties based API (like the one introduced in base/win/shortcut.h) with default values to simplify call sites who just want to create a basic Chrome shortcut.

shell_util.h:
  (CreateChromeDesktopShortcut, CreateChromeQuickLaunchShortcut, UpdateChromeShortcut) => CreateOrUpdateChromeShortcut

  (GetDesktopPath, GetQuickLaunchPath) => GetShortcutPath

  (GetChromeShortcutName) => ChromeShortcutProperties::set_shortcut_name() (i.e. default name is GetAppShortcutName() unless set otherwise... special profile shortcut appending logic moved to ProfileShortcutManager::GetShortcutNameForProfile).

  (RemoveChromeDesktopShortcut, RemoveChromeDesktopShortcutsWithAppendedNames) => RemoveChromeShortcut

install.h:
  (CreateOrUpdateStartMenuAndTaskbarShortcuts, CreateOrUpdateDesktopAndQuickLaunchShortcuts) => CreateOrUpdateShortcuts (and incorporate some of the logic previously handled directly in InstallOrUpdateProduct()).

Finally thoroughly tests shell_util and install shortcut methods!

BUG=148539
TEST=Install chrome at user and system level with and without create_all_shortcuts master pref and make sure it works as intended in all scenarios.
installer_util_unittests.exe --gtest_filter=ShellUtilShortcutTest*
setup_unittests.exe --gtest_filter=InstallShortcutTest*
unit_tests.exe --gtest_filter=ProfileShortcutManagerTest*

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

[gab, 2012-08-17 20:53:24]

Here are the code changes, I'll follow up with the test adjustments in this CL,
but in a later patchset.

Cheers,
Gab

[gab, 2012-08-21 21:48:32]

Uploaded test changes. I'll make another CL for the much needed file_util
shortcut test upgrade.

[robertshield, 2012-08-22 21:39:06]

Did a quick pass through for style and such. The shortcut creation scenarios are
complicated and I'm not sure they all make sense to me yet. I'll go back and
read the doc, have a drink and look through this again soon.

http://codereview.chromium.org/10836247/diff/6002/chrome/installer/setup/inst...
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/10836247/diff/6002/chrome/installer/setup/inst...
chrome/installer/setup/install.cc:284: dist->GetIconIndex(), options |
ShellUtil::SHORTCUT_PIN_TO_TASKBAR)) {
one arg per line please

http://codereview.chromium.org/10836247/diff/6002/chrome/installer/setup/inst...
chrome/installer/setup/install.cc:298: if
(!ShellUtil::CreateOrUpdateChromeShortcut(
one arg per line please

http://codereview.chromium.org/10836247/diff/6002/chrome/installer/setup/inst...
chrome/installer/setup/install.cc:327: ShellUtil::SHORTCUT_DESKTOP,
one arg per line please

http://codereview.chromium.org/10836247/diff/6002/chrome/installer/setup/inst...
chrome/installer/setup/install.cc:338: string16(), chrome_exe.value(),
dist->GetIconIndex(), options)) {
ditto

http://codereview.chromium.org/10836247/diff/6002/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10836247/diff/6002/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:197: const string16& appended_name,
almost all callers of this pass string16() as the argument for |description| and
|appended_name|. Please could you rename this to
CreateOrUpdateChromeShortcutWithDescription() and create a new
CreateOrUpdateChromeShortcut() that does not take |description| and
|appended_name|?

http://codereview.chromium.org/10836247/diff/7017/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10836247/diff/7017/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:268: !ShellUtil::RemoveChromeShortcut(
I suspect this will cause the second call to RemoveChromeShortcut to not happen
if the first one succeeds

http://codereview.chromium.org/10836247/diff/7017/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10836247/diff/7017/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1004: *path = path->AppendASCII("Application
Data");
This doesn't look like the right way of getting these paths. I think what you
want is to use the shell functions to find the user's app data dir. See
SHGetKnownFolderPath
(http://msdn.microsoft.com/en-us/library/windows/desktop/bb762188(v=vs.85).aspx)

http://codereview.chromium.org/10836247/diff/7017/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1013: *path = path->AppendASCII("Quick
Launch");
Same comment about not hard-coding paths. Doing this makes Raymond Chen cry:
http://blogs.msdn.com/b/oldnewthing/archive/2003/11/03/55532.aspx

http://codereview.chromium.org/10836247/diff/7017/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1601: ret = ret &&
file_util::Delete(system_shortcut_path, false);
if ret is already false, the Delete operation is never called

[gab, 2012-10-02 02:44:58]

Applied your comments and some more over 8 CLs :), see http://crbug.com/148539.

PTAL, this now includes unit tests and end-to-end tests for mostly all of the
shortcut stuff :)!

Cheers!
Gab

[gab, 2012-10-02 03:46:21]

Updated CL description for guidance in your review.

Thanks!
Gab

[robertshield, 2012-10-02 13:08:03]

Just a couple of comments from a quick pass, will do a more thorough review Real
Soon Now

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:132:
ShellUtil::ChromeShortcutProperties default_properties;
This is slightly less readable than the previous code. Before, it was explicit
that we were working on user-level shortcuts, now it isn't unless other code has
been read and understood.

Suggest getting rid of the 0-param constructor for ChromeShortcutProperties and
instead adding a constructor that takes system/user level as a parameter.

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:153: NOTREACHED();
we don't check the return value of GetDistribution in most places (we assume it
can never return null).

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1090: BrowserDistribution* dist,
this doesn't use the dist parameter

[grt (UTC plus 2), 2012-10-02 13:22:30]

very preliminary drive-by part 1.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:955: static const wchar_t lnk_ext[] =
L".lnk";
although the style guide doesn't call it out, i think this constant should
either be kLnkExt at the top of this function, or kLnkExt at the top of the
namespace.  i prefer the former.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:979: bool target_is_chrome_exe = false;
i wonder if there's a better way to handle default values for Chrome.  let's
discuss in person.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:985: if (properties.target.BaseName() ==
FilePath(installer::kChromeExe))
BaseName().value() == installer.kChromeExe to avoid an unnecessary temporary

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:998:
shortcut_properties.set_icon(properties.icon, 0);
the icon index can't be set by users of ChromeShortcutProperties?

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1006: installer::MasterPreferences prefs(
this scares me a little bit.  let's talk about it.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:40: SHORTCUT_DESKTOP = 0,
personal preference: remove " = 0".

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:49: // present on this machine.
" on this machine" is superfluous

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:51: // Overwrite an existing shortcut (fails
if the shortcut doesn't exist).
fails -> fail

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:58: // shortcut on creation/update. On
update, non-set properties are ignored;
"non-set" -> "unset" here and below

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:75: : options(0U), system_level(false),
pin_to_taskbar(false) {}
be sure to list all members of POD type, and always put them in the same order
as they're declared in the class:

: dual_mode(false), system_level(false), pin_to_taskbar(false), options(0U)

the following is equivalent since default initialization leads to
zero-initialization for these types:

: dual_mode(), system_level(), pin_to_taskbar(), options()

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:120: // The target to launch from this
shortcut. This is mandatory when creating
since callers are expected to use the functions, i think it's more useful to put
these doc strings up on the setters above and put a "// See comments above for
details." here.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:126: // The localized description of the
shortcut. If this is not set, the
I think it's nice if the same language is used to describe the default for all
of these.  For example:

// The whozawhatsit. The default value is blah.
or
// The whozawhatsit. If some condition, the default value is blah.

For example:

// The localized description of the shortcut. If |target| is chrome.exe, the
default value is the description for the current browser distribution.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:263: // Returns false on failure (in which
case |path| shouldn't to be trusted).
does the "trust" statement mean that |path| might be modified even on failure? 
if so, i think "|path| may be modified on failure" is more clear.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:274: // |appended_name| provides a string to
be appended to the distribution name
while you're changing this, how about:
// |appended_name| provides a (possibly empty) string to be appended to the
distribution name.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:470: // |properties|: Only
properties.system_level and properties.shortcut_name are
i think system_level and shortcut_name should be passed independently
(|properties| seems like overkill).

[gab, 2012-10-03 15:14:59]

Comments addressed -- PTAL.

Thanks!
Gab

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:132:
ShellUtil::ChromeShortcutProperties default_properties;
On 2012/10/02 13:08:04, robertshield wrote:
> This is slightly less readable than the previous code. Before, it was explicit
> that we were working on user-level shortcuts, now it isn't unless other code
has
> been read and understood.
> 
> Suggest getting rid of the 0-param constructor for ChromeShortcutProperties
and
> instead adding a constructor that takes system/user level as a parameter.

Done (and added ShellChange instead of "bool system_level" as an argument
throughout the remainder of the interface changes).

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_shortcut_manager_win.cc (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_shortcut_manager_win.cc:153: NOTREACHED();
On 2012/10/02 13:08:04, robertshield wrote:
> we don't check the return value of GetDistribution in most places (we assume
it
> can never return null). 

True, removed!

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:955: static const wchar_t lnk_ext[] =
L".lnk";
On 2012/10/02 13:22:30, grt wrote:
> although the style guide doesn't call it out, i think this constant should
> either be kLnkExt at the top of this function, or kLnkExt at the top of the
> namespace.  i prefer the former.

Adding L".lnk" in a bunch of places has been bugging me throughout this CL,
finally added it to util_constants.h.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:979: bool target_is_chrome_exe = false;
On 2012/10/02 13:22:30, grt wrote:
> i wonder if there's a better way to handle default values for Chrome.  let's
> discuss in person.

As discussed the target is now always chrome.exe when using
CreateOrUpdateChromeShortcut (the only call site not using chrome.exe, i.e.
uninstall link pointing to setup.exe now uses
base::win::CreateOrUpdateShortcutLink() directly).

The default values are still handled here (I considered making them part of the
constructor of ChromeShortcutProperties, but that would make the constructor
non-trivial).

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:985: if (properties.target.BaseName() ==
FilePath(installer::kChromeExe))
On 2012/10/02 13:22:30, grt wrote:
> BaseName().value() == installer.kChromeExe to avoid an unnecessary temporary

I also debated doing that and decided not to given we are really comparing
FilePaths which should know better how to compare paths than a pure
case-sensitive string comparison (as it is now FilePaths comparison is only
case-insensitive on the drive letter, but this could arguably be made better on
Windows if ever required).

I could make this a base::strcasecmp(properties.target.BaseName().value(),
installer::kChromeExe) == 0, but this implies knowledge of Windows paths'
case-insensitivity here and thus this is logic I believe belongs in
FilePath::operator==, not here.

I don't want to change FilePath::operator== as part of this CL, but am happy to
do so in another CL preceding this one.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:998:
shortcut_properties.set_icon(properties.icon, 0);
On 2012/10/02 13:22:30, grt wrote:
> the icon index can't be set by users of ChromeShortcutProperties?

No current caller required it, i.e. either callers set a .ico file (and thus use
0 as an icon_index) or they would use dist->GetIconIndex() when the target was
chrome.exe (which, if icon_index was specified in master_preferences, would be
overriden by the master_prefs value in the old UpdateChromeShortcut() method).

Thus I feel adding it as a ChromeShortcutProperties just adds noise for a never
used scenario. With the current structure it's fairly easy to add it later if
required anyways.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1006: installer::MasterPreferences prefs(
On 2012/10/02 13:22:30, grt wrote:
> this scares me a little bit.  let's talk about it.

Yea it's ugly, just copying the old behavior from the deleted
UpdateChromeShortcut() method (see end of this diff) for now.

Not sure why we would ever overwrite this in master_prefs..? Do some
distribution deals have their own icons? I would prefer always using
dist->GetIconIndex()...

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1090: BrowserDistribution* dist,
On 2012/10/02 13:08:04, robertshield wrote:
> this doesn't use the dist parameter

Oops, I added this with the intent of removing GetDistribution() on line 1120
below (done now).
I debated going either way, but figured that most callers already knew |dist|
and that GetDistribution() was a non-trivial getter so that it might as well get
passed in (on the other hand it's not always needed which would argue in favor
of keeping the GetDistribution() below and removing the |dist| parameter).

I'm fine either way... your thoughts sirs?!

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:40: SHORTCUT_DESKTOP = 0,
On 2012/10/02 13:22:30, grt wrote:
> personal preference: remove " = 0".

in src/chrome/* 230 instances use " = 0". 474 don't... so you win :)

Done.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:49: // present on this machine.
On 2012/10/02 13:22:30, grt wrote:
> " on this machine" is superfluous

Done.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:51: // Overwrite an existing shortcut (fails
if the shortcut doesn't exist).
On 2012/10/02 13:22:30, grt wrote:
> fails -> fail

Done.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:58: // shortcut on creation/update. On
update, non-set properties are ignored;
On 2012/10/02 13:22:30, grt wrote:
> "non-set" -> "unset" here and below

Done.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:75: : options(0U), system_level(false),
pin_to_taskbar(false) {}
On 2012/10/02 13:22:30, grt wrote:
> be sure to list all members of POD type, and always put them in the same order
> as they're declared in the class:
> 
> : dual_mode(false), system_level(false), pin_to_taskbar(false), options(0U)
> 
> the following is equivalent since default initialization leads to
> zero-initialization for these types:
> 
> : dual_mode(), system_level(), pin_to_taskbar(), options()

Done.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:120: // The target to launch from this
shortcut. This is mandatory when creating
On 2012/10/02 13:22:30, grt wrote:
> since callers are expected to use the functions, i think it's more useful to
put
> these doc strings up on the setters above and put a "// See comments above for
> details." here.

Ya I was also debating doing that, makes sense, done.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:126: // The localized description of the
shortcut. If this is not set, the
On 2012/10/02 13:22:30, grt wrote:
> I think it's nice if the same language is used to describe the default for all
> of these.  For example:
> 
> // The whozawhatsit. The default value is blah.
> or
> // The whozawhatsit. If some condition, the default value is blah.
> 
> For example:
> 
> // The localized description of the shortcut. If |target| is chrome.exe, the
> default value is the description for the current browser distribution.

Agreed, I'll switch to use a specific syntax for each default so that it's easy
to see what are the defaults in a quick glance.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:263: // Returns false on failure (in which
case |path| shouldn't to be trusted).
On 2012/10/02 13:22:30, grt wrote:
> does the "trust" statement mean that |path| might be modified even on failure?


This used to be the case, but no longer is, removing this part of the comment.
Thanks!

> if so, i think "|path| may be modified on failure" is more clear.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:274: // |appended_name| provides a string to
be appended to the distribution name
On 2012/10/02 13:22:30, grt wrote:
> while you're changing this, how about:
> // |appended_name| provides a (possibly empty) string to be appended to the
> distribution name.

Oops, this description was out of date, this method no longer takes these params
in this patch set.

http://codereview.chromium.org/10836247/diff/23001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:470: // |properties|: Only
properties.system_level and properties.shortcut_name are
On 2012/10/02 13:22:30, grt wrote:
> i think system_level and shortcut_name should be passed independently
> (|properties| seems like overkill).

Done, I was using properties to avoid having NULLs all over the place for
|shortcut_name|, but I agree that using properties is equally confusing and
overkill (given most properties are irrelevant).

[robertshield, 2012-10-04 00:25:23]

Looking really good. I like this a lot better than what we had before. A few
more comments.

http://codereview.chromium.org/10836247/diff/30001/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_win.cc (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_win.cc:114:
ShellUtil::ChromeShortcutProperties properties(ShellUtil::CURRENT_USER);
nit: vague preference for shortcut_properties over properties here and below.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:90: message.append(" shortcut to ");
shouldn't this line be inside the if below?

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:354: ShellUtil::ChromeShortcutOperation
operation =
please call this shortcut_operation instead.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:363: base_properties.set_dual_mode(true);
imo some lines of whitespace here would make this easier to read. As is, I find
it to be an impenetrable wall of text :)

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:394: // INSTALL_SHORTCUT_CREATE_MANDATORY.
I find the comment above unclear. Perhaps something like:

// Override operation if install_operation is INSTALL_SHORTCUT_CREATE_MANDATORY

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
File chrome/installer/setup/install_unittest.cc (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install_unittest.cc:239: TEST_F(InstallShortcutTest,
CreateAllShortcuts) {
nice tests :)

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:90: // Default for chrome.exe: the current
distribution's description.
Isn't this in fact the default for SHORTCUT_CREATE_ALWAYS or
SHORTCUT_REPLACE_EXISTING?

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:91: // The length of this string must be less
than MAX_PATH.
Where is this MAX_PATH restriction enforced? Should it be DCHECKed here?

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:457: // no shortcut is found at |location|).
For this last sentence, how about:
"Returns true on success or if no shortcut is found at |location|."

[gab, 2012-10-04 04:10:32]

Thanks! PTAL

http://codereview.chromium.org/10836247/diff/30001/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_win.cc (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_win.cc:114:
ShellUtil::ChromeShortcutProperties properties(ShellUtil::CURRENT_USER);
On 2012/10/04 00:25:23, robertshield wrote:
> nit: vague preference for shortcut_properties over properties here and below.

Sure, although this makes the definition take 2 lines...

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:90: message.append(" shortcut to ");
On 2012/10/04 00:25:23, robertshield wrote:
> shouldn't this line be inside the if below?

No, there is always a |chrome_exe| (in CREATE and REPLACE cases which are the
ones logged here), but it doesn't always have |arguments|.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:354: ShellUtil::ChromeShortcutOperation
operation =
On 2012/10/04 00:25:23, robertshield wrote:
> please call this shortcut_operation instead.

Done.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:363: base_properties.set_dual_mode(true);
On 2012/10/04 00:25:23, robertshield wrote:
> imo some lines of whitespace here would make this easier to read. As is, I
find
> it to be an impenetrable wall of text :)

Done.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install.cc:394: // INSTALL_SHORTCUT_CREATE_MANDATORY.
On 2012/10/04 00:25:23, robertshield wrote:
> I find the comment above unclear. Perhaps something like:
> 
> // Override operation if install_operation is
INSTALL_SHORTCUT_CREATE_MANDATORY

Done and simplified all the other comments, let me know what you think.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
File chrome/installer/setup/install_unittest.cc (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/setup/ins...
chrome/installer/setup/install_unittest.cc:239: TEST_F(InstallShortcutTest,
CreateAllShortcuts) {
On 2012/10/04 00:25:23, robertshield wrote:
> nice tests :)

Thanks :)!

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:90: // Default for chrome.exe: the current
distribution's description.
On 2012/10/04 00:25:23, robertshield wrote:
> Isn't this in fact the default for SHORTCUT_CREATE_ALWAYS or
> SHORTCUT_REPLACE_EXISTING?

Whoops forgot "for chrome.exe" on this one, removed.

Clarified comment at the top of the struct about defaults to say this is also
true on replace.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:91: // The length of this string must be less
than MAX_PATH.
On 2012/10/04 00:25:23, robertshield wrote:
> Where is this MAX_PATH restriction enforced? Should it be DCHECKed here?

MAX_PATH is enforced in base::win::shortcut.h, I brought the comment here
because I felt it wasn't right for the caller to have to read the underlying
interface, I guess I should also bring the DCHECK to avoid someone having the
same question later (and to make sure we catch dev-errors asap).

Done.

http://codereview.chromium.org/10836247/diff/30001/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:457: // no shortcut is found at |location|).
On 2012/10/04 00:25:23, robertshield wrote:
> For this last sentence, how about:
> "Returns true on success or if no shortcut is found at |location|."

Sure.

[grt (UTC plus 2), 2012-10-04 14:31:22]

rubberstamp lgtm

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1100: if (dir_key == -1 ||
!PathService::Get(dir_key, path) || path->empty()) {
dir_key will never be -1 here due to line 1097, no?

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:62: PROPERTIES_CHROME_EXE = 1 << 0,
you could add inline has_foo methods to make checking for these easier for the
callers.  for example:
  bool has_chrome_exe() const {
    return options & PROPERTIES_CHROME_EXE;
  }
i think this can reduce wrapping (and need for braces) and increase clarity in a
number of places.

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:127: // Sets whether this is a dual mode
shortcut (Win8+).
as discussed: please add to this comment some text explaining that this should
only be set for start menu shortcuts to Chrome the browser when it has the
default app id and no arguments.

[robertshield, 2012-10-04 15:41:51]

LGTM

[gab, 2012-10-04 16:39:23]

Addressed grt's comments.

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1100: if (dir_key == -1 ||
!PathService::Get(dir_key, path) || path->empty()) {
On 2012/10/04 14:31:22, grt wrote:
> dir_key will never be -1 here due to line 1097, no?

As discussed, the point was to make it clear it has to be set if a new case was
to be added above, but as you mentioned PahtService.Get(-1,..) will return false
anyways...

Removed.

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:62: PROPERTIES_CHROME_EXE = 1 << 0,
On 2012/10/04 14:31:22, grt wrote:
> you could add inline has_foo methods to make checking for these easier for the
> callers.  for example:
>   bool has_chrome_exe() const {
>     return options & PROPERTIES_CHROME_EXE;
>   }
> i think this can reduce wrapping (and need for braces) and increase clarity in
a
> number of places.

Done.

http://codereview.chromium.org/10836247/diff/27003/chrome/installer/util/shel...
chrome/installer/util/shell_util.h:127: // Sets whether this is a dual mode
shortcut (Win8+).
On 2012/10/04 14:31:22, grt wrote:
> as discussed: please add to this comment some text explaining that this should
> only be set for start menu shortcuts to Chrome the browser when it has the
> default app id and no arguments.

Done, also made a logic change to now only set dual mode on the Start menu
shortcut (as we discussed it doesn't make sense for any other shortcut than the
default Chrome shortcut to be dual mode; furthermore the hopes that setting it
on the Desktop shortcut would help make it a tile if pinned to Start turned out
to be wrong as the appid gets clobbered by Windows when pinned to Start so that
it doesn't become a tile anyways).

[gab, 2012-10-04 16:42:51]

+sail for chrome/browser/profiles/* OWNER approval
+jhawkins for OWNER rubberstamp for side-effects on
chrome/browser/chrome_browser_main_win.cc and
chrome/browser/first_run/first_run_win.cc

Thanks!
Gab

[James Hawkins, 2012-10-04 16:44:47]

My files lgtm

[sail, 2012-10-04 16:51:34]

profiles/* LGTM
looks much nicer now!

[gab, 2012-10-04 18:40:13]

On 2012/10/04 16:51:34, sail wrote:
> profiles/* LGTM
> looks much nicer now!

Thanks!

[commit-bot: I haz the power, 2012-10-04 18:40:27]

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

[commit-bot: I haz the power, 2012-10-04 20:18:29]

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

[commit-bot: I haz the power, 2012-10-05 01:34:07]

Change committed as 160284