Windows: Remove desktop profile shortcuts (and any others pointing to the exe) on uninstall.

Windows: Remove desktop profile shortcuts (and any others pointing to the exe) on uninstall.

BUG=146636
TEST=Uninstalling Chrome removes any present profile shortcuts on the desktop.

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

[gab, 2013-01-02 15:27:02]

This actually does more than the CL description says (i.e. it's not only the
profile shortcuts, but literally any shortcuts (user-created included) present
in the known shortcuts location and pointing to target_exe that is deleted).
Could you update the CL description to reflect this?

Otherwise, lgtm w/ a single comment nit.

Cheers!
Gab

https://codereview.chromium.org/11693010/diff/11/chrome/installer/util/shell_...
File chrome/installer/util/shell_util.h (right):

https://codereview.chromium.org/11693010/diff/11/chrome/installer/util/shell_...
chrome/installer/util/shell_util.h:500: // their targets are |target_exe|.
s/targets are/target is 
(every given shortcut only has 1 target).

[Alexei Svitkine (slow), 2013-01-02 16:06:58]

I forgot to update chrome/installer/setup/uninstall.cc in the previous patch.
Can you take another look to review the changes in that file?

Thanks!

https://codereview.chromium.org/11693010/diff/11/chrome/installer/util/shell_...
File chrome/installer/util/shell_util.h (right):

https://codereview.chromium.org/11693010/diff/11/chrome/installer/util/shell_...
chrome/installer/util/shell_util.h:500: // their targets are |target_exe|.
On 2013/01/02 15:27:02, gab wrote:
> s/targets are/target is 
> (every given shortcut only has 1 target).

Done.

[gab, 2013-01-02 16:12:49]

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

https://codereview.chromium.org/11693010/diff/21001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1066: const FilePath chrome_exe_path(
We have an item on the installer fixit list
(https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...)
already to convert all of these string16 paths to FilePaths; not sure I like the
in between state here -- i.e. in the ideal world all of these variables would
still be name *_exe (not *_exe_path; I don't like the unnecessary extended
variable name).

For now what about leaving chrome_exe as a string16 and inlining
FilePath(chrome_exe) below -- as it is only needed once for now anyways?

https://codereview.chromium.org/11693010/diff/21001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1137: const FilePath app_host_exe_path(
I'd prefer keeping the "app_host_exe" name here.

[Alexei Svitkine (slow), 2013-01-02 16:15:25]

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

https://codereview.chromium.org/11693010/diff/21001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1066: const FilePath chrome_exe_path(
On 2013/01/02 16:12:49, gab wrote:
> We have an item on the installer fixit list
>
(https://docs.google.com/a/google.com/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1fH...)
> already to convert all of these string16 paths to FilePaths; not sure I like
the
> in between state here -- i.e. in the ideal world all of these variables would
> still be name *_exe (not *_exe_path; I don't like the unnecessary extended
> variable name).
> 
> For now what about leaving chrome_exe as a string16 and inlining
> FilePath(chrome_exe) below -- as it is only needed once for now anyways?

Done.

https://codereview.chromium.org/11693010/diff/21001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:1137: const FilePath app_host_exe_path(
On 2013/01/02 16:12:49, gab wrote:
> I'd prefer keeping the "app_host_exe" name here.

Done.

[gab, 2013-01-02 16:16:17]

lgtm

[Alexei Svitkine (slow), 2013-01-02 16:17:07]

Thanks.

+sky for chrome/browser OWNERS

[Alexei Svitkine (slow), 2013-01-02 19:07:14]

(sky doesn't seem to be around right now...)

-sky
+brett

Brett, could you take a look for chrome/ OWNERS approval? Thanks.

[brettw, 2013-01-02 22:16:16]

lgtm

[commit-bot: I haz the power, 2013-01-02 22:17:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/11693010/30003

[commit-bot: I haz the power, 2013-01-03 00:09:26]

Change committed as 174893