On user-level Chrome self-destruct, make user-created shortcuts target system-level chrome.exe.

On user-level Chrome self-destruct, make user-created shortcuts target system-level chrome.exe.

The old behavior was to delete all the shortcuts, which is rather user-hostile. With this change, the user-created shortcuts will be made to target the new system-level Chrome, to form a better transition experience.

- To determine whether or not a shortcut (to the user-level chrome.exe) is user-created, the heuristic we use is to test that the arguments of the shortcut are empty.
- The default shortcut to user-level chrome.exe is NOT retargeted, so it would get deleted (thus avoiding duplicate shortcut to the system-level Chrome).

Right now the "Chrome Apps" is not handled. This is a similar issue as http://crbug.com/238895 , and should be handled in a different CL.

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

[huangs, 2013-08-09 21:17:10]

This is a preliminary CL. Currently Start Screen links are not migrated. This
because these shortcuts reside in 

C:\Users\[username]\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Chrome
Apps

but the current flow only looks into:

C:\Users\[username]\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Google
Chrome

I think the true solution lies in
https://chromiumcodereview.appspot.com/15255004/ (currently these shortcuts are
also not deleted on uninstall). But perhaps I can check in a special case to get
this into M30?

PTAL.

[grt (UTC plus 2), 2013-08-13 03:40:59]

Hi Sam. Who are you asking to take a look at this? Is it ready for review?

[huangs, 2013-08-14 18:19:39]

This is not ready for review yet; I'll have to merge with recent refactoring
changes by calamity@.

[grt (UTC plus 2), 2013-08-15 02:52:27]

On 2013/08/14 18:19:39, huangs1 wrote:
> This is not ready for review yet; I'll have to merge with recent refactoring
> changes by calamity@.

Groovy.

[huangs, 2013-08-23 15:48:14]

Synched with https://chromiumcodereview.appspot.com/15255004/ . But turns out I
still need https://codereview.chromium.org/13864015/
.

I think I should just ignore the "Chrome Apps" migration problem for now, since
it's the same issue as http://crbug.com/238895 (Uninstalling Chrome should
remove app shortcuts).

If this is okay, then I wish to begin review; PTAL.

[huangs, 2013-08-29 14:38:48]

Ping on review. Thanks.

[grt (UTC plus 2), 2013-09-04 03:50:36]

hi sam. looks pretty good. the goal here is for profile and app shortcuts to
continue to work after the user to system handoff?

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

https://codereview.chromium.org/22382007/diff/12001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:333: void RetargetShortcuts(const
InstallerState& installer_state,
since this only makes sense for a self-destruct, which only happens for
user-level installs, i don't think the function needs the flexibility to be
called otherwise. i seem to recall that one of the shortcut paths cannot even be
obtained from the path service when running as SYSTEM, so some of the code below
might actually do the wrong thing if someone mistakenly tried calling this
function for a system-level install. please consider adding
DCHECK(!installer_state.system_install()); and possibly going so far as to
remove the conditional on line 339.

https://codereview.chromium.org/22382007/diff/12001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1198: // system-level chrome.exe. Heuristic:
consider only shortcuts tha have
tha -> that

[huangs, 2013-09-04 14:48:39]

Yes, it's to migrated the shortcuts -- a feature that gab@ requested. Also
updated the CL comments. PTAL.

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

https://codereview.chromium.org/22382007/diff/12001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:333: void RetargetShortcuts(const
InstallerState& installer_state,
Done. Also added blurb on function comment.

https://codereview.chromium.org/22382007/diff/12001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1198: // system-level chrome.exe. Heuristic:
consider only shortcuts tha have
On 2013/09/04 03:50:36, grt wrote:
> tha -> that

Done.

[grt (UTC plus 2), 2013-09-04 15:01:04]

lgtm, but please update the description: the "prerequesite CL" appears to have
landed, so mention of it is confusing.

[huangs, 2013-09-04 21:37:16]

Synched, updated comment and committing!

[commit-bot: I haz the power, 2013-09-04 21:37:39]

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

[grt (UTC plus 2), 2013-09-04 23:24:38]

On 2013/09/04 21:37:16, huangs1 wrote:
> Synched, updated comment and committing!

I meant that "This CL builds on top of the refactoring CL
https://chromiumcodereview.appspot.com/22607006/" is confusing since that link
is for a CR that's already landed. I don't think that comment is necessary.

[huangs, 2013-09-04 23:26:15]

Ah, okay. Removed.

[commit-bot: I haz the power, 2013-09-05 02:53:49]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-09-05 04:21:54]

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

[commit-bot: I haz the power, 2013-09-05 04:22:14]

Change committed as 221343

[gab, 2013-09-09 13:00:31]

Sorry for the late review, last week was nuts... Comments below, please address
in a follow-up CL.

Thanks!
Gab

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

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:332: // |require_args| is set, then only
updates shortcuts with non-empty targets.
s/non-empty targets/non-empty arguments

?

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:340:
DCHECK(!installer_state.system_install());
Move DCHECK to top: checking the method's contract via DCHECKs should always be
done at the top of the method.

Also consider using:

if (!installer_state.system_install()) {
  NOTREACHED();
  return;
}

to be safer (as not everybody runs the installer in a Debug build before
committing...).

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:346: if
(!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist,
Please put these in a for loop through all locations (same for DeleteShortcuts
below).

The only advantage not to be in a loop seems to be the error logging.

Perhaps this calls for
ShellUtil::GetShortcutLocationString(ShellUtil::ShortcutLocation location) {}
? I think they are other places this could be used (e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/s...).

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/util/she...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/util/she...
chrome/installer/util/shell_util.cc:2077: bool require_args,
I don't see a case where setting |require_args| to false for UpdateShortcuts()
makes sense... how about hardcoding it hardcoded to |false| below.

I find this whole |require_args| argument kind of confusing (I understand now it
is to re-use the same filter for Delete and Update) and would rather at least
keep it internal to ShellUtil.

To test it instead you simply have to make sure that calling RemoveShortcuts
removes all shortcuts (including those without args); while UpdateShortcuts only
updates those with args.

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/util/she...
File chrome/installer/util/shell_util_unittest.cc (right):

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/util/she...
chrome/installer/util/shell_util_unittest.cc:526: chrome_exe_, /*require_args*/
true, updated_properties));
nit: I'm not against the inline comment, but I'd put it after |true|.

https://codereview.chromium.org/22382007/diff/24001/chrome/installer/util/she...
chrome/installer/util/shell_util_unittest.cc:540: chrome_exe_, /*require_args*/
false, updated_properties));
Same here.

[huangs, 2013-09-09 15:03:46]

Okay, will make follow-up issue.

[huangs, 2013-09-11 19:02:51]

Follow up in https://codereview.chromium.org/23581012

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/s...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:332: // |require_args| is set, then only
updates shortcuts with non-empty targets.
Ah, good catch. Done.

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:340:
DCHECK(!installer_state.system_install());
Done (should get rid of "!").

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:346: if
(!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist,
DeleteShortcuts() message string also distinguish between "delete" and "unpin".

I was planning to have this as a separate refactoring CL, after calamity@'s
https://chromiumcodereview.appspot.com/13864015/  has been committed.

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/u...
chrome/installer/util/shell_util.cc:2077: bool require_args,
The purpose of |require_args| is keep UpdateShortcuts() general; it's rather ad
hoc that our main usage opts to not retarget links without args (so we will not
have duplicate shortcuts). 

To follow he suggestion, I'm removing |require_args|, and renaming
UpdateShortcuts() to UpdateShortcutsWithArgs() to be explicit. The test cases
would also need to be changed (dummy arguments need to be added).

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/u...
File chrome/installer/util/shell_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/u...
chrome/installer/util/shell_util_unittest.cc:526: chrome_exe_, /*require_args*/
true, updated_properties));
On 2013/09/09 13:00:31, gab wrote:
> nit: I'm not against the inline comment, but I'd put it after |true|.

I'm putting it in front for resemblance with Python with its named argument. But
the |require_args| param is being removed anyway.

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/u...
chrome/installer/util/shell_util_unittest.cc:540: chrome_exe_, /*require_args*/
false, updated_properties));
On 2013/09/09 13:00:31, gab wrote:
> Same here.

Done.

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

Thanks :)! Will continue there...

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/s...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:346: if
(!ShellUtil::UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_DESKTOP, dist,
On 2013/09/11 19:02:52, huangs1 wrote:
> DeleteShortcuts() message string also distinguish between "delete" and
"unpin".
> 
> I was planning to have this as a separate refactoring CL, after calamity@'s
> https://chromiumcodereview.appspot.com/13864015/  has been committed.

Ah ok, sgtm :)!

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/22382007/diff/24001/chrome/installer/u...
chrome/installer/util/shell_util.cc:2077: bool require_args,
On 2013/09/11 19:02:52, huangs1 wrote:
> The purpose of |require_args| is keep UpdateShortcuts() general; it's rather
ad
> hoc that our main usage opts to not retarget links without args (so we will
not
> have duplicate shortcuts). 
> 
> To follow he suggestion, I'm removing |require_args|, and renaming
> UpdateShortcuts() to UpdateShortcutsWithArgs() to be explicit. 

sgtm.

> The test cases
> would also need to be changed (dummy arguments need to be added).