Remove packaged app Windows shortcuts when app is uninstalled.

Remove packaged app Windows shortcuts when app is uninstalled.

BUG=130456
TEST=Check app shortcuts are removed when the app is uninstalled. Test extension uninstallation in general.


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

[benwells, 2012-08-01 06:42:24]

mihai for general review
brettw as owner for chrome/ and base/

I took the opportunity to clean up the web_app code a little.

[benwells, 2012-08-03 03:30:04]

On 2012/08/01 06:42:24, benwells wrote:
> mihai for general review
> brettw as owner for chrome/ and base/
> 
> I took the opportunity to clean up the web_app code a little.

ping

[brettw, 2012-08-05 05:24:25]

https://chromiumcodereview.appspot.com/10837034/diff/2001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://chromiumcodereview.appspot.com/10837034/diff/2001/base/file_util_win....
base/file_util_win.cc:369: bool GetShortcutArguments(const FilePath&
shortcut_path, string16* args) {
There's too much copy-and-pasted code here from the previous function (including
a comment that doesn't even apply here). This should either be one function or
the two should be refactored somehow.

[brettw, 2012-08-05 17:55:24]

https://chromiumcodereview.appspot.com/10837034/diff/2001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://chromiumcodereview.appspot.com/10837034/diff/2001/base/file_util_win....
base/file_util_win.cc:369: bool GetShortcutArguments(const FilePath&
shortcut_path, string16* args) {
Maybe we should add a function to return a ScopedCOMPtr<IShellLink> with a given
shortcut path loaded into it. Then you can implement the GetArguments logic
yourself in your own code (since this is only called once, I'm not sure we need
a base function for it) and you can also implement ResolveShortcut in terms of
this function.

[benwells, 2012-08-07 12:24:05]

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

http://codereview.chromium.org/10837034/diff/2001/base/file_util_win.cc#newco...
base/file_util_win.cc:369: bool GetShortcutArguments(const FilePath&
shortcut_path, string16* args) {
On 2012/08/05 17:55:24, brettw wrote:
> Maybe we should add a function to return a ScopedCOMPtr<IShellLink> with a
given
> shortcut path loaded into it. Then you can implement the GetArguments logic
> yourself in your own code (since this is only called once, I'm not sure we
need
> a base function for it) and you can also implement ResolveShortcut in terms of
> this function.

OK, that all makes sense. For now I've merged these two into one function, and
restructured a bit (also errors in GetPath and GetArguments, while unlikely, are
now handled).

I'm not sure if I like this or your suggestion for a function to return a loaded
shell link. I thought this would feel better but now I'm not so sure - if
someone else ever wants some other shortcut field (e.g. description)  having it
the way you suggest would be better.

[brettw, 2012-08-07 17:51:27]

base LGTM, I didn't look at the rest.

http://codereview.chromium.org/10837034/diff/10001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/10837034/diff/10001/base/file_util_win.cc#newc...
base/file_util_win.cc:350: // Query IShellLink for the IPersistFile interface
While you're modifying these, can you make sure there are periods at the end of
the comments?

[benwells, 2012-08-08 01:17:12]

+sky for chrome/ and ui/ OWNER
+rdsmith for net/ OWNER

mihai: ping for general approach

[sky, 2012-08-08 04:42:41]

I only looked at c/b/u, which LGTM

[Randy Smith (Not in Mondays), 2012-08-08 15:49:36]

With the below comment, url_request_file_job.cc LGTM.

http://codereview.chromium.org/10837034/diff/14001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10837034/diff/14001/base/file_util.h#newcode242
base/file_util.h:242: // if all requested fields are are found successfully.
Would you explicitly indicate here that it's ok for |target_path ==
&shortcut_path| and make sure there's a test for that case?  (I haven't checked
the tests to see if there already is.)

[Mihai Parparita -not on Chrome, 2012-08-09 01:09:47]

LGTM

https://chromiumcodereview.appspot.com/10837034/diff/14001/chrome/browser/ext...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

https://chromiumcodereview.appspot.com/10837034/diff/14001/chrome/browser/ext...
chrome/browser/extensions/app_shortcut_manager.cc:146: delete_info.extension_id
= extension->id();
Nit: This seems to repeat the initialization done in
InstallApplicationShortcuts. Can you move it to a helper function?

https://chromiumcodereview.appspot.com/10837034/diff/14001/chrome/browser/web...
File chrome/browser/web_applications/web_app_win.cc (right):

https://chromiumcodereview.appspot.com/10837034/diff/14001/chrome/browser/web...
chrome/browser/web_applications/web_app_win.cc:71: std::vector<FilePath>
GetShortcutPaths(bool create_on_desktop,
Having so many boolean params makes code harder to read. An enum is most likely
overkill though. Can you just pass in ShortcutInfo (and in
DeletePlatformShortcuts, override the create flags to always make them true)?

[benwells, 2012-08-10 07:10:00]

http://codereview.chromium.org/10837034/diff/14001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/10837034/diff/14001/base/file_util.h#newcode242
base/file_util.h:242: // if all requested fields are are found successfully.
On 2012/08/08 15:49:36, rdsmith wrote:
> Would you explicitly indicate here that it's ok for |target_path ==
> &shortcut_path| and make sure there's a test for that case?  (I haven't
checked
> the tests to see if there already is.)

Done.

http://codereview.chromium.org/10837034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/app_shortcut_manager.cc (right):

http://codereview.chromium.org/10837034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/app_shortcut_manager.cc:146: delete_info.extension_id
= extension->id();
On 2012/08/09 01:09:47, Mihai Parparita wrote:
> Nit: This seems to repeat the initialization done in
> InstallApplicationShortcuts. Can you move it to a helper function?

Done.

http://codereview.chromium.org/10837034/diff/14001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_win.cc (right):

http://codereview.chromium.org/10837034/diff/14001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:71: std::vector<FilePath>
GetShortcutPaths(bool create_on_desktop,
On 2012/08/09 01:09:47, Mihai Parparita wrote:
> Having so many boolean params makes code harder to read. An enum is most
likely
> overkill though. Can you just pass in ShortcutInfo (and in
> DeletePlatformShortcuts, override the create flags to always make them true)?

Done.

[commit-bot: I haz the power, 2012-08-10 07:10:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10837034/10003

[commit-bot: I haz the power, 2012-08-10 07:45:44]

Try job failure for 10837034-10003 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-10 09:53:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10837034/11034

[commit-bot: I haz the power, 2012-08-10 12:50:13]

Change committed as 151021