Windows: When an app is updated and its name changes, recreate shortcuts.

Windows: When an app is updated and its name changes, recreate shortcuts.

This searches for shortcuts in the usual locations containing the app's old
name, deletes them, and creates new shortcuts in the places where there were
pre-existing shortcuts.

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

[benwells, 2013-05-16 04:30:59]

Sam, can you take a first pass through this?

https://codereview.chromium.org/14993013/diff/7001/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/14993013/diff/7001/chrome/common/extensions/e...
chrome/common/extensions/extension.h:680: InstalledExtensionInfo(const
Extension* extension, bool is_update,
nit: one parameter per line.

[Matt Giuca, 2013-05-16 06:40:23]

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:352: // will not be renamed, but
at least it will continue to work.
This is not ideal, but I want to leave it as-is for now, and I will file a
separate low-priority bug about it. The consequences of not fixing this are
simply that if the user right-clicks or tooltips the pinned shortcut, they will
see the old name. I don't consider this as important as the Start Menu shortcut,
where the text is shown directly to the user.

[koz (OOO until 15th September), 2013-05-17 01:36:18]

Random drive-by nits for you :-)

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2434: if (existing_extension !=
NULL) {
nit: Just "if (existing_extension) {". It's cleaner.

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:134: // created. If
|pin_to_taskbar| is true, also pins a shortcut to the taskbar.
There is no |pin_to_taskbar| here.

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:223: // Deletes all shortcuts for
an app, recording the paths at they were found.
"the paths at they were found" -> "the paths they were found at"

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:225: // in |shortcut_info|).
Could you elaborate slightly on why |title| isn't necessarily the title found in
|shortcut_info|?

[Matt Giuca, 2013-05-20 01:02:46]

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2434: if (existing_extension !=
NULL) {
On 2013/05/17 01:36:19, koz wrote:
> nit: Just "if (existing_extension) {". It's cleaner.

Done.

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:134: // created. If
|pin_to_taskbar| is true, also pins a shortcut to the taskbar.
On 2013/05/17 01:36:19, koz wrote:
> There is no |pin_to_taskbar| here.

Thanks, good catch. That comment was from an older iteration of this CL.

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:223: // Deletes all shortcuts for
an app, recording the paths at they were found.
On 2013/05/17 01:36:19, koz wrote:
> "the paths at they were found" -> "the paths they were found at"

Done.

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:225: // in |shortcut_info|).
On 2013/05/17 01:36:19, koz wrote:
> Could you elaborate slightly on why |title| isn't necessarily the title found
in
> |shortcut_info|?

Done.

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:352: // will not be renamed, but
at least it will continue to work.
Actually, I think this is important to do now. If we don't do it now, we won't
be able to do it in the future, since we'll forget the old app name and will
never be able to clean up those shortcuts.

[Matt Giuca, 2013-05-23 02:36:24]

OK, I now re-pin the taskbar shortcuts.

This causes any pinned shortcuts getting renamed to move to the right-most side
of the taskbar, but this is unavoidable. It ONLY happens if the shortcut's name
is changing (not whenever an app updates), so it should barely cause any
problems.

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/7001/chrome/browser/web_applica...
chrome/browser/web_applications/web_app_win.cc:352: // will not be renamed, but
at least it will continue to work.
Done.

[Sam McNally, 2013-05-27 06:24:08]

lgtm

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/extension_service_unittest.cc:724: const std::string&
expect_old_name) {
How about expected_old_name?

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:272: title);
Indentation.

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:396:
DeleteAndRememberShortcuts(web_app_path, shortcut_info, shortcut_info.title,
Consider adding a "DeleteShortcuts" overload.

[Matt Giuca, 2013-05-28 00:41:15]

benwells: I think it's ready for you to look at now.

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/extensions...
chrome/browser/extensions/extension_service_unittest.cc:724: const std::string&
expect_old_name) {
On 2013/05/27 06:24:08, Sam McNally wrote:
> How about expected_old_name?

Done.

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:272: title);
On 2013/05/27 06:24:08, Sam McNally wrote:
> Indentation.

Done.

https://codereview.chromium.org/14993013/diff/41001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:396:
DeleteAndRememberShortcuts(web_app_path, shortcut_info, shortcut_info.title,
On 2013/05/27 06:24:08, Sam McNally wrote:
> Consider adding a "DeleteShortcuts" overload.

I assume you mean adding a function:

DeleteShortcuts(web_app_path, shortcut_info, title) {
  DeleteAndRememberShortcuts(web_app_path, shortcut_info, title, NULL, NULL);
}
?

I don't think it's worth it. It would only be called in one place, and
DeletePlatformShortcuts is almost the same as that function anyway. I can do it
if you think it's necessary, but otherwise I won't.

[Matt Giuca, 2013-05-28 01:09:14]

https://codereview.chromium.org/14993013/diff/62001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/62001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:230: void
GetShortcutLocationsAndDeleteShortcuts(
This was renamed at Sam's request: the new name is clearer about what two
operations this does, and doesn't imply that it "remembers" shortcuts as some
kind of side effect.

Note that we discussed breaking this up into two functions, but decided it
wasn't worth it, as that would involve redundant calls to
MatchingShortcutsForProfileAndExtension.

I also changed the arguments: instead of taking a |shortcut_info|, it now just
takes |profile_path|. This clears up the confusion of the old one, which would
ignore |shortcut_info.title| in favour of the |title| argument.

[benwells, 2013-05-28 02:14:07]

lgtm

https://codereview.chromium.org/14993013/diff/62001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/62001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:230: void
GetShortcutLocationsAndDeleteShortcuts(
On 2013/05/28 01:09:14, Matt Giuca wrote:
> This was renamed at Sam's request: the new name is clearer about what two
> operations this does, and doesn't imply that it "remembers" shortcuts as some
> kind of side effect.
> 
> Note that we discussed breaking this up into two functions, but decided it
> wasn't worth it, as that would involve redundant calls to
> MatchingShortcutsForProfileAndExtension.
> 
> I also changed the arguments: instead of taking a |shortcut_info|, it now just
> takes |profile_path|. This clears up the confusion of the old one, which would
> ignore |shortcut_info.title| in favour of the |title| argument.

Could you add a comment somewhere that duplicates will be deleted but only one
path returned for duplicates in the same folder?

https://codereview.chromium.org/14993013/diff/62001/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/14993013/diff/62001/chrome/common/extensions/...
chrome/common/extensions/extension.h:560: InstalledExtensionInfo(const
Extension* extension, bool is_update,
Nit: one line per parameter.

[Matt Giuca, 2013-05-28 03:10:06]

https://codereview.chromium.org/14993013/diff/62001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/14993013/diff/62001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:230: void
GetShortcutLocationsAndDeleteShortcuts(
On 2013/05/28 02:14:07, benwells wrote:
> On 2013/05/28 01:09:14, Matt Giuca wrote:
> > This was renamed at Sam's request: the new name is clearer about what two
> > operations this does, and doesn't imply that it "remembers" shortcuts as
some
> > kind of side effect.
> > 
> > Note that we discussed breaking this up into two functions, but decided it
> > wasn't worth it, as that would involve redundant calls to
> > MatchingShortcutsForProfileAndExtension.
> > 
> > I also changed the arguments: instead of taking a |shortcut_info|, it now
just
> > takes |profile_path|. This clears up the confusion of the old one, which
would
> > ignore |shortcut_info.title| in favour of the |title| argument.
> 
> Could you add a comment somewhere that duplicates will be deleted but only one
> path returned for duplicates in the same folder?

Done.

https://codereview.chromium.org/14993013/diff/62001/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/14993013/diff/62001/chrome/common/extensions/...
chrome/common/extensions/extension.h:560: InstalledExtensionInfo(const
Extension* extension, bool is_update,
On 2013/05/28 02:14:07, benwells wrote:
> Nit: one line per parameter.

Done.

[commit-bot: I haz the power, 2013-05-28 03:10:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14993013/70001

[commit-bot: I haz the power, 2013-05-28 05:29:39]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-28 06:30:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14993013/70001

[commit-bot: I haz the power, 2013-05-28 08:17:32]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-28 08:19:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14993013/70001

[commit-bot: I haz the power, 2013-05-28 10:14:40]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-28 23:50:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14993013/70001

[commit-bot: I haz the power, 2013-05-29 00:36:58]

Change committed as 202712