Restricting scope of ShellUtil's shortcut update feature for its specific usage in Chrome self-dest…

Restricting scope of ShellUtil's shortcut update feature for its specific usage in Chrome self-destruct flow.

This is a follow-up to https://chromiumcodereview.appspot.com/22382007/ , to implement gab@'s suggested changes. Details:
- Renaming ShellUtil::UpdateShortcuts() to ShellUtil::UpdateShortcuts() to UpdateShortcutsWithArgs().
- Removing the |require_args| from the interface of ShellUtil::UpdateShortcuts() (always set to true).
- Change is propagated to caller in uninstall.cc.
- Update to tests.

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

[huangs, 2013-09-11 19:03:15]

See last set of messages in https://chromiumcodereview.appspot.com/22382007

PTAL.

[gab, 2013-09-11 20:34:23]

Thanks, looks good, some comments below.

Cheers!
Gab

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:332: // target |new_target_exe| instead.
This can only be called from user-level.
s/from user-level/for user-level installs

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:341:
DCHECK(!installer_state.system_install());
Remove this line (same as above if).

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:348: if
(!ShellUtil::UpdateShortcutsWithArgs(
Can we at least add a TODO to put these in a for loop after calamity@'s CL? What
blocks us from doing it without his CL? And which CL are we talking about
exactly -- he did land a few recently?

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1192: // Chrome exists.
Restructure this comment to only mention what is happening here (i.e.,
retargeting); the comment below might be sufficient in itself).

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1202: // non-empty args. Therefore the main
user-level chrome.exe will not get
Such detail about the heuristic, etc. belong in the implementation not the call
site.

[huangs, 2013-09-11 21:31:24]

PTAL.

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:332: // target |new_target_exe| instead.
This can only be called from user-level.
On 2013/09/11 20:34:23, gab wrote:
> s/from user-level/for user-level installs

Done.

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:341:
DCHECK(!installer_state.system_install());
Oops. Done.

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:348: if
(!ShellUtil::UpdateShortcutsWithArgs(
I'm thinking of https://chromiumcodereview.appspot.com/13864015/ , which should
add the "Chrome Apps" folder.

Added TODO(huangs).

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1192: // Chrome exists.
On 2013/09/11 20:34:23, gab wrote:
> Restructure this comment to only mention what is happening here (i.e.,
> retargeting); the comment below might be sufficient in itself).

Done.

https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:1202: // non-empty args. Therefore the main
user-level chrome.exe will not get
Done.

[gab, 2013-09-13 01:53:26]

lgtm w/ comments below.

Cheers!
Gab

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:331: 
nit: Remove this extra empty line.

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:336: void RetargetUserShortcuts(const
InstallerState& installer_state,
WDYT of calling this method RetargetUserShortcutsWithArgs to be consistent with
the other method's name (since you are describing the empty args heuristic on
this method anyways and it will always be used that way).

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1196: // Chrome.
I still think the first part of this comment is overdescriptive, i.e. the only
thing this if does is the retargeting; what about:

// If user-level chrome is self-destructing as a result of encountering a
system-level chrome, retarget owned non-default shortcuts (app shortcuts,
profile shortcuts, etc.) to the system-level chrome.

?

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1206: // main shortcuts to user-level
chrome.exe, so it get deleted below.
I'd say the suggested comment above is sufficient in itself; I don't mind
putting here or above, but I don't find have two comments is very useful here.

[huangs, 2013-09-13 04:16:25]

Updated, but waiting for response re. "chrome" vs. "Chrome". 

I'll be in McGill during the day tomorrow, so may commit a bit later.

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:331: 
On 2013/09/13 01:53:26, gab wrote:
> nit: Remove this extra empty line.

Done.

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:336: void RetargetUserShortcuts(const
InstallerState& installer_state,
I was sneaky and used the word "User" to refer to 2 things:
- this updates user-level shortcuts,
- this update "user-created shortcut", and the "With args" part is
implementation detail that the caller should not know or care.

But then you're using "owned non-default shortcut" below. So following your
suggested change.

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1196: // Chrome.
Done. I take that your use of "chrome" (generic goodness) instead of "Chrome"
(the browser) is intentional, but is it appropriate?

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1206: // main shortcuts to user-level
chrome.exe, so it get deleted below.
Removed comment.

[gab, 2013-09-13 14:31:24]

Thanks, one more nit below, re-lgtm

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/23581012/diff/6001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1196: // Chrome.
On 2013/09/13 04:16:26, huangs1 wrote:
> Done. I take that your use of "chrome" (generic goodness) instead of "Chrome"
> (the browser) is intentional, but is it appropriate?

Yes it is intentional, except that it's chrome (the browser) instead of Chrome
(short name for Google Chrome).

https://codereview.chromium.org/23581012/diff/11001/chrome/installer/setup/un...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/23581012/diff/11001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1207: VLOG(1) << "Retarget failed:
system-level Chrome not found.";
s/VLOG(1)/LOG(ERROR)

[huangs, 2013-09-13 20:17:02]

https://codereview.chromium.org/23581012/diff/11001/chrome/installer/setup/un...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/23581012/diff/11001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1207: VLOG(1) << "Retarget failed:
system-level Chrome not found.";
On 2013/09/13 14:31:25, gab wrote:
> s/VLOG(1)/LOG(ERROR)

Done.

[commit-bot: I haz the power, 2013-09-13 20:18:11]

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

[gab, 2013-09-13 20:26:29]

Looks like the latest diff of uninstall.cc is broken... stopping CQ for now.

[huangs, 2013-09-13 20:32:03]

Weird; thanks for noticing this. Gonna reupload.

[commit-bot: I haz the power, 2013-09-13 20:43:47]

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

[commit-bot: I haz the power, 2013-09-13 23:50:44]

Change committed as 223173