Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1289333005:
Change shortcut install location to non-subdir. (Closed)
DescriptionChange shortcut install location to non-subdir.
BUG=169669
Committed: https://crrev.com/c5bab94fdde60b2bc9ed93822f35c5ce50202d26
Cr-Commit-Position: refs/heads/master@{#356371}
Committed: https://crrev.com/f440aa1a1e31c13ffc981612f50f330e944e2bb4
Cr-Commit-Position: refs/heads/master@{#360867}
Patch Set 1 #Patch Set 2 : fixed tests #Patch Set 3 : add some log messages to figure out test problems #Patch Set 4 : more logging #Patch Set 5 : even more logging #Patch Set 6 : added start-menu-root to LogShortcutOperation #Patch Set 7 : removed test logging #Patch Set 8 : handle update migration from folder to root shortcut #Patch Set 9 : added test for shortcut migration #
Total comments: 5
Patch Set 10 : added new tests #Patch Set 11 : move shortcut instead of delete/create #
Total comments: 12
Patch Set 12 : addressed review comments; re-ordered some params #
Total comments: 8
Patch Set 13 : test improvements #
Total comments: 25
Patch Set 14 : rebased #Patch Set 15 : addressed comments; added tests; removed extraneous parameter #Patch Set 16 : added menu_root to cleanup code #
Total comments: 25
Patch Set 17 : addressed review comments by grt #Patch Set 18 : address review comments by gab #Patch Set 19 : git cl format #
Total comments: 2
Patch Set 20 : undo git cl format #Patch Set 21 : deprecate chrome_subdir in another place #Patch Set 22 : some 'git cl format' changes #
Total comments: 5
Patch Set 23 : added todo; removed blank line #Patch Set 24 : rebased #Patch Set 25 : revert part of cl/1438793002 that is no longer needed #Messages
Total messages: 71 (12 generated)
bcwhite@chromium.org changed reviewers: + gab@chromium.org
Seems like this is all that is necessary. I installed on a fresh VM and "Google Chrome" shows up directly under "All Programs" with no created sub-directory.
On 2015/08/14 22:03:23, bcwhite wrote: > Seems like this is all that is necessary. I installed on a fresh VM and "Google > Chrome" shows up directly under "All Programs" with no created sub-directory. For new installs yes, the tricky part comes in the migration of existing installs. I think we'll want, on update, to move all existing shortcuts out of SHORTCUT_LOCATION_START_MENU_CHROME_DIR into SHORTCUT_LOCATION_START_MENU_ROOT and then delete the folder. We'll also need to remove all code that installs into SHORTCUT_LOCATION_START_MENU_CHROME_DIR. Thanks, Gab
On 2015/08/18 01:22:54, gab wrote: > On 2015/08/14 22:03:23, bcwhite wrote: > > Seems like this is all that is necessary. I installed on a fresh VM and > "Google > > Chrome" shows up directly under "All Programs" with no created sub-directory. > > For new installs yes, the tricky part comes in the migration of existing > installs. > > I think we'll want, on update, to move all existing shortcuts out of > SHORTCUT_LOCATION_START_MENU_CHROME_DIR into SHORTCUT_LOCATION_START_MENU_ROOT > and then delete the folder. I'll verify that. It looks like update always deletes shortcuts from all potential locations and creates it in one. I'll double-check plus try it in the VM. > We'll also need to remove all code that installs into > SHORTCUT_LOCATION_START_MENU_CHROME_DIR. Okay. I'll do that last once everything else is verified.
On 2015/08/24 13:19:50, bcwhite wrote: > On 2015/08/18 01:22:54, gab wrote: > > On 2015/08/14 22:03:23, bcwhite wrote: > > > Seems like this is all that is necessary. I installed on a fresh VM and > > "Google > > > Chrome" shows up directly under "All Programs" with no created > sub-directory. > > > > For new installs yes, the tricky part comes in the migration of existing > > installs. > > > > I think we'll want, on update, to move all existing shortcuts out of > > SHORTCUT_LOCATION_START_MENU_CHROME_DIR into SHORTCUT_LOCATION_START_MENU_ROOT > > and then delete the folder. > > I'll verify that. It looks like update always deletes shortcuts from all > potential locations and creates it in one. I'll double-check plus try it in the > VM. Not quite actually, update uses INSTALL_SHORTCUT_REPLACE_EXISTING which merely replaces existing shortcuts under their specific known install location/name (but if it's now looking for it in a new location it will both not drop the new one -- no existing shortcut to replace -- and not remove the old one). There is indeed code that does a full sweep of all shortcuts that point to the current chrome.exe and updates them, but this is merely to update a few properties (e.g., app id) of existing shortcuts that somehow have it wrong (FWIW, I think this code is too aggressive and should be changed to only occur on OS upgrade when properties tend to require an update). Let me know if this is not clear. Cheers, Gab > > > > We'll also need to remove all code that installs into > > SHORTCUT_LOCATION_START_MENU_CHROME_DIR. > > Okay. I'll do that last once everything else is verified.
> > I'll verify that. It looks like update always deletes shortcuts from all > > potential locations and creates it in one. I'll double-check plus try it in > the > > VM. > > Not quite actually, update uses INSTALL_SHORTCUT_REPLACE_EXISTING which merely > replaces existing shortcuts under their specific known install location/name > (but if it's now looking for it in a new location it will both not drop the new > one -- no existing shortcut to replace -- and not remove the old one). > > There is indeed code that does a full sweep of all shortcuts that point to the > current chrome.exe and updates them, but this is merely to update a few > properties (e.g., app id) of existing shortcuts that somehow have it wrong Good to know. > (FWIW, I think this code is too aggressive and should be changed to only occur > on OS upgrade when properties tend to require an update). By "this code", you're referring to _my_ code or the existing full-weep-of-shortcuts code?
On 2015/08/25 00:39:58, bcwhite wrote: > > > I'll verify that. It looks like update always deletes shortcuts from all > > > potential locations and creates it in one. I'll double-check plus try it in > > the > > > VM. > > > > Not quite actually, update uses INSTALL_SHORTCUT_REPLACE_EXISTING which merely > > replaces existing shortcuts under their specific known install location/name > > (but if it's now looking for it in a new location it will both not drop the > new > > one -- no existing shortcut to replace -- and not remove the old one). > > > > There is indeed code that does a full sweep of all shortcuts that point to the > > current chrome.exe and updates them, but this is merely to update a few > > properties (e.g., app id) of existing shortcuts that somehow have it wrong > > Good to know. > > > > (FWIW, I think this code is too aggressive and should be changed to only occur > > on OS upgrade when properties tend to require an update). > > By "this code", you're referring to _my_ code or the existing > full-weep-of-shortcuts code? The existing MigrateChromiumShortcuts() slightly delayed task happening every startup which results in the full-weep-of-shortcuts-update (nothing to do with your code, only a parallel as to the state of the shortcut code).
Okay, getting back to this... I've added code to detect the migration case and do an explicit remove of the old and force-create of the new. I've verified this on a VM; now looking to write a test.
On 2015/09/16 19:32:14, bcwhite wrote: > Okay, getting back to this... > > I've added code to detect the migration case and do an explicit remove of the > old and force-create of the new. I've verified this on a VM; now looking to > write a test. Test added. Okay to go?
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... chrome/installer/setup/install.cc:442: if (!ExecuteAndLogShortcutOperation( how does this interact with pins in the start screen/menu and taskbar? i don't claim to understand shortcut code too much, but it seems safer to me to move the existing shortcut from the "Google Chrome" folder (if the shortcut exists) up a level to the main Start Menu\Programs folder. i would hope that Windows' link tracking service will keep pins of it working. especially on Win10, where we can't recreate the taskbar pin if the user had created it.
Sorry for the delay, perf+vacation brought a long no code review session... https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... chrome/installer/setup/install.cc:442: if (!ExecuteAndLogShortcutOperation( On 2015/09/22 20:28:53, grt wrote: > how does this interact with pins in the start screen/menu and taskbar? i don't > claim to understand shortcut code too much, but it seems safer to me to move the > existing shortcut from the "Google Chrome" folder (if the shortcut exists) up a > level to the main Start Menu\Programs folder. i would hope that Windows' link > tracking service will keep pins of it working. especially on Win10, where we > can't recreate the taskbar pin if the user had created it. Taskbar pinning creates a copy of the pinned shortcut and is only linked to the original shortcut per its appid (but any shortcut with the same appid will do the same, i.e. show "unpin from taskbar" in its context menu). Not so sure about Win10 start menu pins though. Moving sounds better either way, but requires adding move logic to ShellUtil, perhaps something like ShellUtil::MoveShortcuts(current_location, destination_location, dist) which would move shortcuts in |current_location| to |destination_location| IFF the destination path of each shortcut doesn't exist, otherwise only deleting the original shortcut (i.e. no overwrite), then you can call that method unconditionally here. https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... chrome/installer/setup/install.cc:444: start_menu_properties, shortcut_operation)) { If someone runs an over-install (re-install when existing install is present), this call will succeed creating a shortcut in SHORTCUT_LOCATION_START_MENU_ROOT and then won't proceed to cleaning up the existing shortcut (nor will it on subsequent updates as updating the shortcut in root will work). https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:381: TEST_F(InstallShortcutTest, MigrateExisting) { Other test cases are: - MigrateNonExisting - InstallOldVersionOverNewVersion (i.e. migration on install instead of on update) Perhaps a parametrized test will work best for this, i.e. one bool param (exists in sub-dir) and one InstallShortcutOperation param (another bool param for system vs user-level would be good too). (ref. TEST_P macro: https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...)
> > how does this interact with pins in the start screen/menu and taskbar? i don't > > claim to understand shortcut code too much, but it seems safer to me to move > the > > existing shortcut from the "Google Chrome" folder (if the shortcut exists) up > a > > level to the main Start Menu\Programs folder. i would hope that Windows' link > > tracking service will keep pins of it working. especially on Win10, where we > > can't recreate the taskbar pin if the user had created it. I've been testing this on both Win7 and Win10 (VMs). In all cases, the Start and Taskbar pins remain intact and active while the All-Programs shortcut migrates out of a folder and into the main list. When I started verifying this two weeks ago, I found cases where the sub-folder wasn't removed but doing it all again today I'm not having any problems at all. But I'll check it a third time. > Taskbar pinning creates a copy of the pinned shortcut and is only linked to the > original shortcut per its appid (but any shortcut with the same appid will do > the same, i.e. show "unpin from taskbar" in its context menu). Right, so a copy should remain valid even after the original is removed. Only if it was a shortcut to the original would it fail but then it would fail even if the original was moved rather than deleted and recreated. > Moving sounds better either way, but requires adding move logic to ShellUtil, Still think this is worth the effort? > https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... > chrome/installer/setup/install.cc:444: start_menu_properties, > shortcut_operation)) { > If someone runs an over-install (re-install when existing install is present), > this call will succeed creating a shortcut in SHORTCUT_LOCATION_START_MENU_ROOT > and then won't proceed to cleaning up the existing shortcut (nor will it on > subsequent updates as updating the shortcut in root will work). This is what I've been doing with my testing: an older MSI install followed by a patched mini_installer with "--system-level" (as admin). As long as setup detects an existing install, it gets processed as an update and should migrate. I'll try from an older mini to a patched version in case that makes a difference. > chrome/installer/setup/install_unittest.cc:381: TEST_F(InstallShortcutTest, > MigrateExisting) { > Other test cases are: > - MigrateNonExisting > - InstallOldVersionOverNewVersion (i.e. migration on install instead of on > update) > > Perhaps a parametrized test will work best for this, i.e. one bool param (exists > in sub-dir) and one InstallShortcutOperation param (another bool param for > system vs user-level would be good too). > > (ref. TEST_P macro: > https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...) I'll look into it.
> wasn't removed but doing it all again today I'm not having any problems at all. > But I'll check it a third time. The problem of the shortcut appearing both in the root and in a subfolder only occurs when installing the same version without and then with the patch. Any actual upgrade in version number causes it to work correctly. That's not a problem in practice since actual releases would always have different version numbers.
https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/160001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:381: TEST_F(InstallShortcutTest, MigrateExisting) { Investigating these... > - MigrateNonExisting This would be the same as the Create* and Replace* tests, depending on how you look at it (exists nowhere or exists at destination). > - InstallOldVersionOverNewVersion (i.e. migration on install instead of on > update) Done.
On 2015/10/08 20:25:18, bcwhite wrote: > > > how does this interact with pins in the start screen/menu and taskbar? i > don't > > > claim to understand shortcut code too much, but it seems safer to me to move > > the > > > existing shortcut from the "Google Chrome" folder (if the shortcut exists) > up > > a > > > level to the main Start Menu\Programs folder. i would hope that Windows' > link > > > tracking service will keep pins of it working. especially on Win10, where we > > > can't recreate the taskbar pin if the user had created it. > > I've been testing this on both Win7 and Win10 (VMs). In all cases, the Start > and Taskbar pins remain intact and active while the All-Programs shortcut > migrates out of a folder and into the main list. > > When I started verifying this two weeks ago, I found cases where the sub-folder > wasn't removed but doing it all again today I'm not having any problems at all. > But I'll check it a third time. > > > > Taskbar pinning creates a copy of the pinned shortcut and is only linked to > the > > original shortcut per its appid (but any shortcut with the same appid will do > > the same, i.e. show "unpin from taskbar" in its context menu). > > Right, so a copy should remain valid even after the original is removed. Only > if it was a shortcut to the original would it fail but then it would fail even > if the original was moved rather than deleted and recreated. My memory is fuzzy here, but if it's working reliably it's telling me this: If the target of a shortcut vanishes, Windows will make an effort to find it the next time the shortcut is resolved. When a file is moved, however, Windows will automagically fixup shortcuts pointing to it on account of the link tracking service. This is one reason why I think moving is better than deleting and recreating. We also don't want to lose users' customizations of the shortcut (if any).
On 2015/10/09 15:11:36, grt (very slow until Oct 12) wrote: > On 2015/10/08 20:25:18, bcwhite wrote: > > > > how does this interact with pins in the start screen/menu and taskbar? i > > don't > > > > claim to understand shortcut code too much, but it seems safer to me to > move > > > the > > > > existing shortcut from the "Google Chrome" folder (if the shortcut exists) > > up > > > a > > > > level to the main Start Menu\Programs folder. i would hope that Windows' > > link > > > > tracking service will keep pins of it working. especially on Win10, where > we > > > > can't recreate the taskbar pin if the user had created it. > > > > I've been testing this on both Win7 and Win10 (VMs). In all cases, the Start > > and Taskbar pins remain intact and active while the All-Programs shortcut > > migrates out of a folder and into the main list. > > > > When I started verifying this two weeks ago, I found cases where the > sub-folder > > wasn't removed but doing it all again today I'm not having any problems at > all. > > But I'll check it a third time. > > > > > > > Taskbar pinning creates a copy of the pinned shortcut and is only linked to > > the > > > original shortcut per its appid (but any shortcut with the same appid will > do > > > the same, i.e. show "unpin from taskbar" in its context menu). > > > > Right, so a copy should remain valid even after the original is removed. Only > > if it was a shortcut to the original would it fail but then it would fail even > > if the original was moved rather than deleted and recreated. > > My memory is fuzzy here, but if it's working reliably it's telling me this: If > the target of a shortcut vanishes, Windows will make an effort to find it the > next time the shortcut is resolved. When a file is moved, however, Windows will > automagically fixup shortcuts pointing to it on account of the link tracking > service. This is one reason why I think moving is better than deleting and > recreating. We also don't want to lose users' customizations of the shortcut (if > any). The destination of all the shortcuts is always "C:\Program Files (x86)\Google\Chrome\Application\chrome.exe" This target is always valid never changes even during upgrades so existing shortcuts will remain valid even when the version changes. My change only affects one of the shortcuts, not the target. Customization of the existing shortcut is a good point, though. Those would be lost. Do people actually customize the shortcuts in subfolders of "All Programs"? When I've done it, it has always been on copies I've made on the desktop or taskbar or pinned to start menu; I don't like messing with originals. I'll look into coding the move.
On 2015/10/09 17:49:49, bcwhite wrote: > On 2015/10/09 15:11:36, grt (very slow until Oct 12) wrote: > > On 2015/10/08 20:25:18, bcwhite wrote: > > > > > how does this interact with pins in the start screen/menu and taskbar? i > > > don't > > > > > claim to understand shortcut code too much, but it seems safer to me to > > move > > > > the > > > > > existing shortcut from the "Google Chrome" folder (if the shortcut > exists) > > > up > > > > a > > > > > level to the main Start Menu\Programs folder. i would hope that Windows' > > > link > > > > > tracking service will keep pins of it working. especially on Win10, > where > > we > > > > > can't recreate the taskbar pin if the user had created it. > > > > > > I've been testing this on both Win7 and Win10 (VMs). In all cases, the > Start > > > and Taskbar pins remain intact and active while the All-Programs shortcut > > > migrates out of a folder and into the main list. > > > > > > When I started verifying this two weeks ago, I found cases where the > > sub-folder > > > wasn't removed but doing it all again today I'm not having any problems at > > all. > > > But I'll check it a third time. > > > > > > > > > > Taskbar pinning creates a copy of the pinned shortcut and is only linked > to > > > the > > > > original shortcut per its appid (but any shortcut with the same appid will > > do > > > > the same, i.e. show "unpin from taskbar" in its context menu). > > > > > > Right, so a copy should remain valid even after the original is removed. > Only > > > if it was a shortcut to the original would it fail but then it would fail > even > > > if the original was moved rather than deleted and recreated. > > > > My memory is fuzzy here, but if it's working reliably it's telling me this: If > > the target of a shortcut vanishes, Windows will make an effort to find it the > > next time the shortcut is resolved. When a file is moved, however, Windows > will > > automagically fixup shortcuts pointing to it on account of the link tracking > > service. This is one reason why I think moving is better than deleting and > > recreating. We also don't want to lose users' customizations of the shortcut > (if > > any). > > The destination of all the shortcuts is always > > "C:\Program Files (x86)\Google\Chrome\Application\chrome.exe" > > This target is always valid never changes even during upgrades so existing > shortcuts will remain valid even when the version changes. My change only > affects one of the shortcuts, not the target. > > Customization of the existing shortcut is a good point, though. Those would be > lost. Do people actually customize the shortcuts in subfolders of "All > Programs"? When I've done it, it has always been on copies I've made on the > desktop or taskbar or pinned to start menu; I don't like messing with originals. > > I'll look into coding the move. Move code complete. Is that what you had in mind?
lg (I also assume you don't have an issue with installing the same version over itself anymore with this approach?) Other comments below. Thanks, Gab PS: Apologies again for the delay, been pretty much offline last week with the scheduling summit. https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:446: // to explicitly remove the old one and force-create the new one. Suggest changing this whole paragraph with something like: // Move Start Menu shortcuts out of the deprecated chrome specific subfolder if it exists. This needs to happen before the shortcut update below or it will fail. https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:454: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_DIR, Rename SHORTCUT_LOCATION_START_MENU_CHROME_DIR to SHORTCUT_LOCATION_START_MENU_CHROME_DIR_DEPRECATED to ensure that all affected call sites are dealt with in this CL (and also for future documentation). https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:456: } Also need to delete the subfolder on success. I suggest changing MoveExistingShortcut() to MigrateShortcuts(old_dir, new_dir, ...). Which would move all existing shortcuts in |old_dir| out to the |new_dir| and then delete |old_dir|. https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:383: void InstallShortcutTest::MigrationTest( s/MigrationTest/MigrateAwayFromDeprecatedStartMenuSubfolderTest/ (or something more precise along those lines) https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:415: } I still think it'd be better to TEST_P to test across all install combinations (INSTALL_SHORTCUT_* and user/system), but I could live with this as a compromise if the other one is too complex.
https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:446: // to explicitly remove the old one and force-create the new one. On 2015/10/13 19:35:09, gab wrote: > Suggest changing this whole paragraph with something like: > > // Move Start Menu shortcuts out of the deprecated chrome specific subfolder if > it exists. This needs to happen before the shortcut update below or it will > fail. Done. https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:454: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_DIR, On 2015/10/13 19:35:09, gab wrote: > Rename SHORTCUT_LOCATION_START_MENU_CHROME_DIR to > SHORTCUT_LOCATION_START_MENU_CHROME_DIR_DEPRECATED to ensure that all affected > call sites are dealt with in this CL (and also for future documentation). Done. https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:456: } On 2015/10/13 19:35:09, gab wrote: > Also need to delete the subfolder on success. > > I suggest changing MoveExistingShortcut() to MigrateShortcuts(old_dir, new_dir, > ...). Which would move all existing shortcuts in |old_dir| out to the |new_dir| > and then delete |old_dir|. Generally there is only the one but should there be others, say "uninstall Chrome", from some long-ago install, should we really move that to the root directory? I think those would be better left under the sub-folder to avoid clutter. https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:383: void InstallShortcutTest::MigrationTest( On 2015/10/13 19:35:09, gab wrote: > s/MigrationTest/MigrateAwayFromDeprecatedStartMenuSubfolderTest/ > > (or something more precise along those lines) Done. https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:415: } On 2015/10/13 19:35:09, gab wrote: > I still think it'd be better to TEST_P to test across all install combinations > (INSTALL_SHORTCUT_* and user/system), but I could live with this as a compromise > if the other one is too complex. Done.
@grt: see question for you below Thanks, Gab https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:456: } On 2015/10/15 18:29:35, bcwhite wrote: > On 2015/10/13 19:35:09, gab wrote: > > Also need to delete the subfolder on success. > > > > I suggest changing MoveExistingShortcut() to MigrateShortcuts(old_dir, > new_dir, > > ...). Which would move all existing shortcuts in |old_dir| out to the > |new_dir| > > and then delete |old_dir|. > > Generally there is only the one but should there be others, say "uninstall > Chrome", from some long-ago install, should we really move that to the root > directory? I think those would be better left under the sub-folder to avoid > clutter. I really think we should aim to delete the subfolder. And thus we should probably move everything (e.g. potential custom shortcuts, etc.) out. I'm not worried about the uninstall link (in fact we removed the cleanup code for it recently, 2 years after its deprecation). And on uninstall we nuke every shortcut that points to chrome.exe in known locations so a simple uninstall should get rid of any weird cruft for unhappy users (actually the uninstall link points to setup.exe but we could also add a rule to clean those up if we care). Edit: I just saw that you added logic to delete the folder if empty after the move (e.g. the common case). Maybe that's actually better. Hmmm, @grt: WDYT? https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:156: base::win::UnpinShortcutFromStart(system_start_menu_subdir_shortcut_); Also need to add matching UnpinShortcutFromTaskbar() calls above. https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:392: ShortcutParams( Constructor goes before members (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...). https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:439: INSTANTIATE_TEST_CASE_P( Add a comment like: // Verify that any installer operation for any installation level triggers the migration. https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:447: ShortcutParams(installer::INSTALL_SHORTCUT_CREATE_ALL, Instead of using a custom struct, make your test a WithParamInterface<std::tr1::tuple<installer::InstallShortcutOperation, installer::InstallShortcutLevel>> (mapping those to protected const members of the MigrateShortcutTest fixture). And then here you can use Combine(Values(INSTALL_SHORTCUT_CREATE_ALL, INSTALL_SHORTCUT_CREATE_EACH_IF_NO_SYSTEM_LEVEL, INSTALL_SHORTCUT_REPLACE_EXISTING), Values(CURRENT_USER, ALL_USERS)) e.g.: https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:156: base::win::UnpinShortcutFromStart(system_start_menu_subdir_shortcut_); On 2015/10/15 18:55:06, gab wrote: > Also need to add matching UnpinShortcutFromTaskbar() calls above. Done. https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:392: ShortcutParams( On 2015/10/15 18:55:06, gab wrote: > Constructor goes before members > (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...). Done. https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:439: INSTANTIATE_TEST_CASE_P( On 2015/10/15 18:55:06, gab wrote: > Add a comment like: > > // Verify that any installer operation for any installation level triggers the > migration. Done. https://codereview.chromium.org/1289333005/diff/220001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:447: ShortcutParams(installer::INSTALL_SHORTCUT_CREATE_ALL, On 2015/10/15 18:55:06, gab wrote: > Instead of using a custom struct, make your test a > WithParamInterface<std::tr1::tuple<installer::InstallShortcutOperation, > installer::InstallShortcutLevel>> (mapping those to protected const members of > the MigrateShortcutTest fixture). > > And then here you can use > Combine(Values(INSTALL_SHORTCUT_CREATE_ALL, > INSTALL_SHORTCUT_CREATE_EACH_IF_NO_SYSTEM_LEVEL, > INSTALL_SHORTCUT_REPLACE_EXISTING), > Values(CURRENT_USER, ALL_USERS)) > > e.g.: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende... Nice. Done.
@grt, good with you?
On 2015/10/19 14:20:32, bcwhite wrote: > @grt, good with you? In particuliar, PTAL @ question on this comment: """ https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:456: } On 2015/10/15 18:29:35, bcwhite wrote: > On 2015/10/13 19:35:09, gab wrote: > > Also need to delete the subfolder on success. > > > > I suggest changing MoveExistingShortcut() to MigrateShortcuts(old_dir, > new_dir, > > ...). Which would move all existing shortcuts in |old_dir| out to the > |new_dir| > > and then delete |old_dir|. > > Generally there is only the one but should there be others, say "uninstall > Chrome", from some long-ago install, should we really move that to the root > directory? I think those would be better left under the sub-folder to avoid > clutter. I really think we should aim to delete the subfolder. And thus we should probably move everything (e.g. potential custom shortcuts, etc.) out. I'm not worried about the uninstall link (in fact we removed the cleanup code for it recently, 2 years after its deprecation). And on uninstall we nuke every shortcut that points to chrome.exe in known locations so a simple uninstall should get rid of any weird cruft for unhappy users (actually the uninstall link points to setup.exe but we could also add a rule to clean those up if we care). Edit: I just saw that you added logic to delete the folder if empty after the move (e.g. the common case). Maybe that's actually better. Hmmm, @grt: WDYT? """
Link doesn't work for me, but the content is: > > I suggest changing MoveExistingShortcut() to MigrateShortcuts(old_dir, > > new_dir, > > > ...). Which would move all existing shortcuts in |old_dir| out to the > > |new_dir| > > > and then delete |old_dir|. > > > > Generally there is only the one but should there be others, say > "uninstall > > Chrome", from some long-ago install, should we really move that to the > root > > directory? I think those would be better left under the sub-folder to > avoid > > clutter. > I really think we should aim to delete the subfolder. And thus we should > probably move everything (e.g. potential custom shortcuts, etc.) out. > I'm not worried about the uninstall link (in fact we removed the cleanup > code > for it recently, 2 years after its deprecation). > And on uninstall we nuke every shortcut that points to chrome.exe in known > locations so a simple uninstall should get rid of any weird cruft for > unhappy > users (actually the uninstall link points to setup.exe but we could also > add a > rule to clean those up if we care). > Edit: I just saw that you added logic to delete the folder if empty after > the > move (e.g. the common case). Maybe that's actually better. Hmmm, @grt: > WDYT? -- Brian https://codereview.chromium.org/1289333005/ On Mon, Oct 19, 2015 at 12:46 PM, <gab@chromium.org> wrote: > On 2015/10/19 14:20:32, bcwhite wrote: > >> @grt, good with you? >> > > In particuliar, PTAL @ question on this comment: > > """ > > > https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup. > .. > > chrome/installer/setup/install.cc:456: } > On 2015/10/15 18:29:35, bcwhite wrote: > >> On 2015/10/13 19:35:09, gab wrote: >> > Also need to delete the subfolder on success. >> > >> > I suggest changing MoveExistingShortcut() to MigrateShortcuts(old_dir, >> new_dir, >> > ...). Which would move all existing shortcuts in |old_dir| out to the >> |new_dir| >> > and then delete |old_dir|. >> > > Generally there is only the one but should there be others, say "uninstall >> Chrome", from some long-ago install, should we really move that to the >> root >> directory? I think those would be better left under the sub-folder to >> avoid >> clutter. >> > > I really think we should aim to delete the subfolder. And thus we should > probably move everything (e.g. potential custom shortcuts, etc.) out. > > I'm not worried about the uninstall link (in fact we removed the cleanup > code > for it recently, 2 years after its deprecation). > And on uninstall we nuke every shortcut that points to chrome.exe in known > locations so a simple uninstall should get rid of any weird cruft for > unhappy > users (actually the uninstall link points to setup.exe but we could also > add a > rule to clean those up if we care). > > Edit: I just saw that you added logic to delete the folder if empty after > the > move (e.g. the common case). Maybe that's actually better. Hmmm, @grt: > WDYT? > > """ > > https://codereview.chromium.org/1289333005/ > -- Brian bcwhite@google.com ----------------------------------------------------------------------------------------- *Treat someone as they are and they will remain that way.Treat someone as they can be and they will become that way.* To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/200001/chrome/installer/setup... chrome/installer/setup/install.cc:456: } On 2015/10/15 18:55:06, gab wrote: > On 2015/10/15 18:29:35, bcwhite wrote: > > On 2015/10/13 19:35:09, gab wrote: > > > Also need to delete the subfolder on success. > > > > > > I suggest changing MoveExistingShortcut() to MigrateShortcuts(old_dir, > > new_dir, > > > ...). Which would move all existing shortcuts in |old_dir| out to the > > |new_dir| > > > and then delete |old_dir|. > > > > Generally there is only the one but should there be others, say "uninstall > > Chrome", from some long-ago install, should we really move that to the root > > directory? I think those would be better left under the sub-folder to avoid > > clutter. > > I really think we should aim to delete the subfolder. And thus we should > probably move everything (e.g. potential custom shortcuts, etc.) out. > > I'm not worried about the uninstall link (in fact we removed the cleanup code > for it recently, 2 years after its deprecation). > And on uninstall we nuke every shortcut that points to chrome.exe in known > locations so a simple uninstall should get rid of any weird cruft for unhappy > users (actually the uninstall link points to setup.exe but we could also add a > rule to clean those up if we care). > > Edit: I just saw that you added logic to delete the folder if empty after the > move (e.g. the common case). Maybe that's actually better. Hmmm, @grt: WDYT? Hmm. I think leaving old custom shortcuts in the directory is probably best. This satisfies the "minimize user surprise" goal of software. I like that the old directory is removed when it's empty. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:122: bool ExecuteAndLogShortcutOperation( this return value is unused. why introduce it? https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:443: // Move start-menu shortcuts out of the deprecated Chrome specific subfolder. is plural correct here? is there ever more than one shortcut to be moved? https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:444: // This needs to happen before the shortcut update below or it will fail due nit: "or it will" -> "or the latter will" https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:445: // to the existing shortcut not being in the location as the new one. suggestion: "the existing shortcut not being in the location as the new one." -> "an old shortcut in the product directory not having been moved into the top-level Programs directory." or something like that. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:392: std::tr1::tuple< nit: use testing::tuple and testing::get<> rather than std::tr1. Google Test has platform-specific juju, and pulls the correct tuple implementation into its own namespace for compatibility (see https://github.com/google/googletest/blob/master/googletest/include/gtest/int...). https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1733: ShellUtil::ShortcutLocation old_location, omit ShellUtil:: https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1757: bool result = !!::MoveFile(old_shortcut_path.value().c_str(), does base::Move (from base/files/file_util.h) work here? https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.h:334: // set |shortcut_level|. please add to the comment that the folder at |old_location| is removed if it is empty after the move. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.h:335: static bool ShellUtil::MoveExistingShortcut( remove ShellUtil:: https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.h:335: static bool ShellUtil::MoveExistingShortcut( could you add a unittest for this?
+1 to grt's comments (+an unsent draft from me this morning) https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:401: std::tr1::get<1>(GetParam()); Make |shortcut_operation_| and |shortcut_level_| protected const members of the MigrateShortcutTest class and initialize them in its constructor's initialization list (that way the tr1 stuff remains an implementation detail).
https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:443: // Move start-menu shortcuts out of the deprecated Chrome specific subfolder. On 2015/10/19 17:28:45, grt wrote: > is plural correct here? is there ever more than one shortcut to be moved? Done. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:444: // This needs to happen before the shortcut update below or it will fail due On 2015/10/19 17:28:45, grt wrote: > nit: "or it will" -> "or the latter will" Done. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:445: // to the existing shortcut not being in the location as the new one. On 2015/10/19 17:28:44, grt wrote: > suggestion: "the existing shortcut not being in the [same] location as the new one." -> > "an old shortcut in the product directory not having been moved into the > top-level Programs directory." or something like that. That would be incorrect. It fails below not because the old shortcut exists at the old location but because the old shortcut does not exist at the new location. I added the missing word, "same" to make this clearer. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:392: std::tr1::tuple< On 2015/10/19 17:28:45, grt wrote: > nit: use testing::tuple and testing::get<> rather than std::tr1. Google Test has > platform-specific juju, and pulls the correct tuple implementation into its own > namespace for compatibility (see > https://github.com/google/googletest/blob/master/googletest/include/gtest/int...). Done. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:401: std::tr1::get<1>(GetParam()); On 2015/10/19 18:00:14, gab wrote: > Make |shortcut_operation_| and |shortcut_level_| protected const members of the > MigrateShortcutTest class and initialize them in its constructor's > initialization list (that way the tr1 stuff remains an implementation detail). Done. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1733: ShellUtil::ShortcutLocation old_location, On 2015/10/19 17:28:45, grt wrote: > omit ShellUtil:: Done. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1757: bool result = !!::MoveFile(old_shortcut_path.value().c_str(), On 2015/10/19 17:28:45, grt wrote: > does base::Move (from base/files/file_util.h) work here? Done. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.h:334: // set |shortcut_level|. On 2015/10/19 17:28:45, grt wrote: > please add to the comment that the folder at |old_location| is removed if it is > empty after the move. Done. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.h:335: static bool ShellUtil::MoveExistingShortcut( On 2015/10/19 17:28:45, grt wrote: > remove ShellUtil:: Done. (here and elsewhere) https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/util/... chrome/installer/util/shell_util.h:335: static bool ShellUtil::MoveExistingShortcut( On 2015/10/19 17:28:45, grt wrote: > could you add a unittest for this? Done.
bcwhite@chromium.org changed reviewers: + jhawkins@chromium.org
jhawkins@chromium.org: Please review changes in chrome/browser (new name for constant)
https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:122: bool ExecuteAndLogShortcutOperation( On 2015/10/19 17:28:44, grt wrote: > this return value is unused. why introduce it? ping https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:445: // to the existing shortcut not being in the location as the new one. On 2015/10/20 16:02:13, bcwhite wrote: > On 2015/10/19 17:28:44, grt wrote: > > suggestion: "the existing shortcut not being in the [same] location as the new > one." -> > > "an old shortcut in the product directory not having been moved into the > > top-level Programs directory." or something like that. > > That would be incorrect. It fails below not because the old shortcut exists at > the old location but because the old shortcut does not exist at the new > location. I added the missing word, "same" to make this clearer. I still find this wording awkward. The call to ExecuteAndLogShortcutOperation will not work as desired with the move taking place first because an existing shortcut hasn't been moved from the old to the new location, correct? "the existing shortcut ... the new one." sounds to me like it's talking about two different shortcuts, which isn't the case. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1734: bool ShellUtil::MoveExistingShortcut( formatting nit: bool ShellUtil::MoveExistingShortcut(ShortcutLocation old_location, ShortcutLocation new_location, BrowserDistribution* dist, const ShortcutProperties& properties) { https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1751: GetShortcutPath( nit: GetShortcutPath(old_location, dist, properties.level, &old_shortcut_path); and below https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.h:328: static bool MoveExistingShortcut( formatting nit: static bool MoveExistingShortcut(ShortcutLocation old_location, ShortcutLocation new_location, BrowserDistribution* dist, const ShortcutProperties& properties);
https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:122: bool ExecuteAndLogShortcutOperation( On 2015/10/20 18:10:30, grt wrote: > On 2015/10/19 17:28:44, grt wrote: > > this return value is unused. why introduce it? > > ping I left it in in because it's not really extra code and could be useful in the future. But I'll remove it. https://codereview.chromium.org/1289333005/diff/240001/chrome/installer/setup... chrome/installer/setup/install.cc:445: // to the existing shortcut not being in the location as the new one. On 2015/10/20 18:10:30, grt wrote: > On 2015/10/20 16:02:13, bcwhite wrote: > > On 2015/10/19 17:28:44, grt wrote: > > > suggestion: "the existing shortcut not being in the [same] location as the > new > > one." -> > > > "an old shortcut in the product directory not having been moved into the > > > top-level Programs directory." or something like that. > > > > That would be incorrect. It fails below not because the old shortcut exists > at > > the old location but because the old shortcut does not exist at the new > > location. I added the missing word, "same" to make this clearer. > > I still find this wording awkward. The call to ExecuteAndLogShortcutOperation > will not work as desired with the move taking place first because an existing > shortcut hasn't been moved from the old to the new location, correct? "the > existing shortcut ... the new one." sounds to me like it's talking about two > different shortcuts, which isn't the case. I see. I think of it as two different shortcuts, one in the root and one in the subfolder so it makes perfect sense to me. I'll reword it. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1734: bool ShellUtil::MoveExistingShortcut( On 2015/10/20 18:10:30, grt wrote: > formatting nit: > bool ShellUtil::MoveExistingShortcut(ShortcutLocation old_location, > ShortcutLocation new_location, > BrowserDistribution* dist, > const ShortcutProperties& properties) { Done. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1751: GetShortcutPath( On 2015/10/20 18:10:30, grt wrote: > nit: > GetShortcutPath(old_location, dist, properties.level, &old_shortcut_path); > and below Done. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.h:328: static bool MoveExistingShortcut( On 2015/10/20 18:10:30, grt wrote: > formatting nit: > static bool MoveExistingShortcut(ShortcutLocation old_location, > ShortcutLocation new_location, > BrowserDistribution* dist, > const ShortcutProperties& properties); Done.
A couple more comments, otherwise this looks very good :-). Thanks! Gab https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:576: APP_MENU_LOCATION_SUBDIR_CHROME, Should also remove APP_MENU_LOCATION_SUBDIR_CHROME -- actually has it been decided where application shortcuts are to go? I thought we were already no longer putting them in the Chrome Start Menu subfolder? https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... chrome/installer/setup/install.cc:117: bool ExecuteAndLogShortcutOperation( grt pinged about this already I think, but adding a comment on this patch set -- why add an unused return value here? https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:133: BrowserDistribution::SUBFOLDER_CHROME)) Should also deprecate BrowserDistribution::SUBFOLDER_CHROME. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:399: }; private: DISALLOW_COPY_AND_ASSIGN(MigrateShortcutTest); (sorry I missed that in previous pass, but all classes in Chromium should have that) https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1746: base::string16 shortcut_name( const https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1759: I'd say rm this empty line. (or otherwise add an empty line between 1760/1761 (as-is 1758 is more related to 1761 then 1760 yet the vertical spacing hints otherwise). https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1765: ShellUtil::ShortcutLocation location, Seems like you're cleaning up those extra namespaces in the header, can they be cleaned here as well? https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.h:328: static bool MoveExistingShortcut( On 2015/10/20 18:10:30, grt wrote: > formatting nit: > static bool MoveExistingShortcut(ShortcutLocation old_location, > ShortcutLocation new_location, > BrowserDistribution* dist, > const ShortcutProperties& properties); Sounds like running "git cl format" will fix a few of the afore-mentioned nits, just making sure you know of that option :-).
https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:576: APP_MENU_LOCATION_SUBDIR_CHROME, On 2015/10/20 18:25:59, gab wrote: > Should also remove APP_MENU_LOCATION_SUBDIR_CHROME -- actually has it been > decided where application shortcuts are to go? I thought we were already no > longer putting them in the Chrome Start Menu subfolder? app_list_service_win.cc:300 (CreateShortcut) This appears to try to create the shortcut on the desktop, in the quick-launch bar, and under the "Google Chrome" subdir. To me this seems correct: user shortcuts shouldn't clutter up the root Start menu; better to have them in a folder. The comments seems to indicate this intent saying that the user "can restore [the taskbar shortcut] by pinning from the start menu" -- I.E. it's not intended for common use. Is there policy to the contrary? https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... chrome/installer/setup/install.cc:117: bool ExecuteAndLogShortcutOperation( On 2015/10/20 18:25:59, gab wrote: > grt pinged about this already I think, but adding a comment on this patch set -- > why add an unused return value here? It was used in a previous incarnation of the patch. I've removed that use and now this return. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:133: BrowserDistribution::SUBFOLDER_CHROME)) On 2015/10/20 18:25:59, gab wrote: > Should also deprecate BrowserDistribution::SUBFOLDER_CHROME. Will do once "app shortcut location" gets resolved. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:399: }; On 2015/10/20 18:25:59, gab wrote: > private: > DISALLOW_COPY_AND_ASSIGN(MigrateShortcutTest); > > > (sorry I missed that in previous pass, but all classes in Chromium should have > that) Done. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1759: On 2015/10/20 18:25:59, gab wrote: > I'd say rm this empty line. > > (or otherwise add an empty line between 1760/1761 (as-is 1758 is more related to > 1761 then 1760 yet the vertical spacing hints otherwise). Done. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.cc:1765: ShellUtil::ShortcutLocation location, On 2015/10/20 18:25:59, gab wrote: > Seems like you're cleaning up those extra namespaces in the header, can they be > cleaned here as well? I was a little hesitant about that. Done. https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/installer/util/... chrome/installer/util/shell_util.h:328: static bool MoveExistingShortcut( On 2015/10/20 18:25:59, gab wrote: > On 2015/10/20 18:10:30, grt wrote: > > formatting nit: > > static bool MoveExistingShortcut(ShortcutLocation old_location, > > ShortcutLocation new_location, > > BrowserDistribution* dist, > > const ShortcutProperties& properties); > > Sounds like running "git cl format" will fix a few of the afore-mentioned nits, > just making sure you know of that option :-). Done.
+cc benwells to comment on the location of the "Chrome App Launcher" shortcut. https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:576: APP_MENU_LOCATION_SUBDIR_CHROME, On 2015/10/20 19:03:12, bcwhite wrote: > On 2015/10/20 18:25:59, gab wrote: > > Should also remove APP_MENU_LOCATION_SUBDIR_CHROME -- actually has it been > > decided where application shortcuts are to go? I thought we were already no > > longer putting them in the Chrome Start Menu subfolder? > > app_list_service_win.cc:300 (CreateShortcut) > This appears to try to create the shortcut on the desktop, in the quick-launch > bar, and under the "Google Chrome" subdir. > > To me this seems correct: user shortcuts shouldn't clutter up the root Start > menu; better to have them in a folder. The comments seems to indicate this > intent saying that the user "can restore [the taskbar shortcut] by pinning from > the start menu" -- I.E. it's not intended for common use. > > Is there policy to the contrary? Are you guys talking about the same thing? The "app list" shortcut is "Chrome App Launcher.lnk", and there can be only one. I think it's okay for this to be in the "Programs" folder. benwells@ may have thoughts. App shortcuts already go into a "Chrome Apps" folder. https://codereview.chromium.org/1289333005/diff/360001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/360001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:568: {creation_locations.on_desktop, ShellUtil::SHORTCUT_LOCATION_DESKTOP}, "git cl format" is very useful, but sometimes does things like this that make the code more ugly. please back this formatting change out. here's my workflow for formatting: - git add <my various changes> - git cl format - git add -p (this gives you an interactive prompt to add or reject the formatting changes.) - git checkout -- . (this wipes out the formatting changes that were rejected) - git commit
Responding below. PS: git cl format -- yea I also use a script that does something similar (and I tend to commit all my stuff first -- instead of just add, just to keep it truly separate): git cl format && git add -up && git co -- * && git diff HEAD && git commit -am "Changes from git cl format" (Note: the double && make the script stop if previous steps fail which is great) https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:576: APP_MENU_LOCATION_SUBDIR_CHROME, On 2015/10/22 17:31:11, grt wrote: > On 2015/10/20 19:03:12, bcwhite wrote: > > On 2015/10/20 18:25:59, gab wrote: > > > Should also remove APP_MENU_LOCATION_SUBDIR_CHROME -- actually has it been > > > decided where application shortcuts are to go? I thought we were already no > > > longer putting them in the Chrome Start Menu subfolder? > > > > app_list_service_win.cc:300 (CreateShortcut) > > This appears to try to create the shortcut on the desktop, in the quick-launch > > bar, and under the "Google Chrome" subdir. > > > > To me this seems correct: user shortcuts shouldn't clutter up the root Start > > menu; better to have them in a folder. The comments seems to indicate this > > intent saying that the user "can restore [the taskbar shortcut] by pinning > from > > the start menu" -- I.E. it's not intended for common use. > > > > Is there policy to the contrary? > > Are you guys talking about the same thing? The "app list" shortcut is "Chrome > App Launcher.lnk", and there can be only one. I think it's okay for this to be > in the "Programs" folder. benwells@ may have thoughts. App shortcuts already go > into a "Chrome Apps" folder. I thought the App Launcher had been phased out, has it not? (or maybe it hasn't been uninstalled, but can we remove the code to install it?)
https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:576: APP_MENU_LOCATION_SUBDIR_CHROME, On 2015/10/22 17:31:11, grt wrote: > On 2015/10/20 19:03:12, bcwhite wrote: > > On 2015/10/20 18:25:59, gab wrote: > > > Should also remove APP_MENU_LOCATION_SUBDIR_CHROME -- actually has it been > > > decided where application shortcuts are to go? I thought we were already no > > > longer putting them in the Chrome Start Menu subfolder? > > > > app_list_service_win.cc:300 (CreateShortcut) > > This appears to try to create the shortcut on the desktop, in the quick-launch > > bar, and under the "Google Chrome" subdir. > > > > To me this seems correct: user shortcuts shouldn't clutter up the root Start > > menu; better to have them in a folder. The comments seems to indicate this > > intent saying that the user "can restore [the taskbar shortcut] by pinning > from > > the start menu" -- I.E. it's not intended for common use. > > > > Is there policy to the contrary? > > Are you guys talking about the same thing? The "app list" shortcut is "Chrome > App Launcher.lnk", and there can be only one. I think it's okay for this to be > in the "Programs" folder. benwells@ may have thoughts. App shortcuts already go > into a "Chrome Apps" folder. Okay, marked deprecated. https://codereview.chromium.org/1289333005/diff/360001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/360001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:568: {creation_locations.on_desktop, ShellUtil::SHORTCUT_LOCATION_DESKTOP}, On 2015/10/22 17:31:11, grt wrote: > "git cl format" is very useful, but sometimes does things like this that make > the code more ugly. please back this formatting change out. > > here's my workflow for formatting: > - git add <my various changes> > - git cl format > - git add -p > (this gives you an interactive prompt to add or reject the formatting > changes.) > - git checkout -- . > (this wipes out the formatting changes that were rejected) > - git commit Done.
@benwells questions for you below. lgtm % nits for this CL assuming a follow-up to clean this up further (see comments below). Thanks, Gab https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:576: APP_MENU_LOCATION_SUBDIR_CHROME, On 2015/10/23 17:22:14, bcwhite wrote: > On 2015/10/22 17:31:11, grt wrote: > > On 2015/10/20 19:03:12, bcwhite wrote: > > > On 2015/10/20 18:25:59, gab wrote: > > > > Should also remove APP_MENU_LOCATION_SUBDIR_CHROME -- actually has it been > > > > decided where application shortcuts are to go? I thought we were already > no > > > > longer putting them in the Chrome Start Menu subfolder? > > > > > > app_list_service_win.cc:300 (CreateShortcut) > > > This appears to try to create the shortcut on the desktop, in the > quick-launch > > > bar, and under the "Google Chrome" subdir. > > > > > > To me this seems correct: user shortcuts shouldn't clutter up the root Start > > > menu; better to have them in a folder. The comments seems to indicate this > > > intent saying that the user "can restore [the taskbar shortcut] by pinning > > from > > > the start menu" -- I.E. it's not intended for common use. > > > > > > Is there policy to the contrary? > > > > Are you guys talking about the same thing? The "app list" shortcut is "Chrome > > App Launcher.lnk", and there can be only one. I think it's okay for this to be > > in the "Programs" folder. benwells@ may have thoughts. App shortcuts already > go > > into a "Chrome Apps" folder. > > Okay, marked deprecated. Actually not convinced it should be deprecated, rather I think it should be removed. But that depends on that status of the App Launcher shortcut -- @benwells? https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/app_list/win/app_list_service_win.cc:309: web_app::APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED; This feels wrong. i.e. either we keep a deprecated location around for removal only (like we're doing for the other one) or we don't deprecate it. As mentioned on the other comment, I think this code should be removed (IIRC App Launcher is no longer a thing?). https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.h (right): https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:81: APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED, Despite my other comments, I think I'm okay letting this CL go in as-is % nits but I would like a TODO here like: // TODO(bcwhite): The Chrome subdir is deprecated, phase out its WebApp usage. https://codereview.chromium.org/1289333005/diff/420001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/420001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:390: rm empty line
https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.h (right): https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:81: APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED, On 2015/10/23 18:00:23, gab wrote: > Despite my other comments, I think I'm okay letting this CL go in as-is % nits > but I would like a TODO here like: > > // TODO(bcwhite): The Chrome subdir is deprecated, phase out its WebApp usage. Done. https://codereview.chromium.org/1289333005/diff/420001/chrome/installer/setup... File chrome/installer/setup/install_unittest.cc (right): https://codereview.chromium.org/1289333005/diff/420001/chrome/installer/setup... chrome/installer/setup/install_unittest.cc:390: On 2015/10/23 18:00:23, gab wrote: > rm empty line Done.
bcwhite@chromium.org changed reviewers: + benwells@chromium.org
+ benwells What should be done with APP_MENU_LOCATION_SUBDIR_CHROME?
On 2015/10/23 18:09:48, bcwhite wrote: > + benwells > > What should be done with APP_MENU_LOCATION_SUBDIR_CHROME? Sorry I was on an offsite last week and I need to investigate before answering the questions on this CL ... I'll look more today and respond.
On 2015/10/23 18:00:23, gab wrote: > @benwells questions for you below. > > lgtm % nits for this CL assuming a follow-up to clean this up further (see > comments below). > > Thanks, > Gab > > https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... > File chrome/browser/web_applications/web_app_win.cc (right): > > https://codereview.chromium.org/1289333005/diff/300001/chrome/browser/web_app... > chrome/browser/web_applications/web_app_win.cc:576: > APP_MENU_LOCATION_SUBDIR_CHROME, > On 2015/10/23 17:22:14, bcwhite wrote: > > On 2015/10/22 17:31:11, grt wrote: > > > On 2015/10/20 19:03:12, bcwhite wrote: > > > > On 2015/10/20 18:25:59, gab wrote: > > > > > Should also remove APP_MENU_LOCATION_SUBDIR_CHROME -- actually has it > been > > > > > decided where application shortcuts are to go? I thought we were already > > no > > > > > longer putting them in the Chrome Start Menu subfolder? > > > > > > > > app_list_service_win.cc:300 (CreateShortcut) > > > > This appears to try to create the shortcut on the desktop, in the > > quick-launch > > > > bar, and under the "Google Chrome" subdir. > > > > > > > > To me this seems correct: user shortcuts shouldn't clutter up the root > Start > > > > menu; better to have them in a folder. The comments seems to indicate > this > > > > intent saying that the user "can restore [the taskbar shortcut] by pinning > > > from > > > > the start menu" -- I.E. it's not intended for common use. > > > > > > > > Is there policy to the contrary? > > > > > > Are you guys talking about the same thing? The "app list" shortcut is > "Chrome > > > App Launcher.lnk", and there can be only one. I think it's okay for this to > be > > > in the "Programs" folder. benwells@ may have thoughts. App shortcuts already > > go > > > into a "Chrome Apps" folder. > > > > Okay, marked deprecated. > > Actually not convinced it should be deprecated, rather I think it should be > removed. > > But that depends on that status of the App Launcher shortcut -- @benwells? The App Launcher is still a thing. I'll send y'all more details offline. I'd say that the App Launcher is kind of a peer of Chrome, so should go in the same place as Chrome. So I agree with gab@, in that APP_MENU_LOCATION_SUBDIR_CHROME should be removed and the callsite updated to use APP_MENU_LOCATION_ROOT (or whatever it is). > > https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/ui/view... > File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): > > https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/ui/view... > chrome/browser/ui/views/app_list/win/app_list_service_win.cc:309: > web_app::APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED; > This feels wrong. i.e. either we keep a deprecated location around for removal > only (like we're doing for the other one) or we don't deprecate it. > > As mentioned on the other comment, I think this code should be removed (IIRC App > Launcher is no longer a thing?). > > https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/web_app... > File chrome/browser/web_applications/web_app.h (right): > > https://codereview.chromium.org/1289333005/diff/420001/chrome/browser/web_app... > chrome/browser/web_applications/web_app.h:81: > APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED, > Despite my other comments, I think I'm okay letting this CL go in as-is % nits > but I would like a TODO here like: > > // TODO(bcwhite): The Chrome subdir is deprecated, phase out its WebApp usage. > > https://codereview.chromium.org/1289333005/diff/420001/chrome/installer/setup... > File chrome/installer/setup/install_unittest.cc (right): > > https://codereview.chromium.org/1289333005/diff/420001/chrome/installer/setup... > chrome/installer/setup/install_unittest.cc:390: > rm empty line
lgtm w/ comment below and follow-up for App Launcher shortcut move. Thanks! Gab https://codereview.chromium.org/1289333005/diff/460001/chrome/browser/ui/view... File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): https://codereview.chromium.org/1289333005/diff/460001/chrome/browser/ui/view... chrome/browser/ui/views/app_list/win/app_list_service_win.cc:309: web_app::APP_MENU_LOCATION_ROOT; Should also move existing shortcut, not just change creation site. I'd say leave it on APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED here for now and do a follow-up CL to move the App Launcher shortcut and change its creation site. Thanks
Patchset #24 (id:460001) has been deleted
> https://codereview.chromium.org/1289333005/diff/460001/chrome/browser/ui/view... > File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): > > https://codereview.chromium.org/1289333005/diff/460001/chrome/browser/ui/view... > chrome/browser/ui/views/app_list/win/app_list_service_win.cc:309: > web_app::APP_MENU_LOCATION_ROOT; > Should also move existing shortcut, not just change creation site. > > I'd say leave it on APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED here for now and > do a follow-up CL to move the App Launcher shortcut and change its creation > site. Okay. Last patch removed (that was the only change in it).
On 2015/10/26 22:46:48, bcwhite wrote: > > > https://codereview.chromium.org/1289333005/diff/460001/chrome/browser/ui/view... > > File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right): > > > > > https://codereview.chromium.org/1289333005/diff/460001/chrome/browser/ui/view... > > chrome/browser/ui/views/app_list/win/app_list_service_win.cc:309: > > web_app::APP_MENU_LOCATION_ROOT; > > Should also move existing shortcut, not just change creation site. > > > > I'd say leave it on APP_MENU_LOCATION_SUBDIR_CHROME_DEPRECATED here for now > and > > do a follow-up CL to move the App Launcher shortcut and change its creation > > site. > > Okay. Last patch removed (that was the only change in it). Ben, are you okay with this? I need your approval for 4 files.
I'm only an owner for three of those files, but they lgtm
On 2015/10/27 00:30:22, benwells wrote: > I'm only an owner for three of those files, but they lgtm James, could I get your approval for the last file: chrome_browser_main_win.cc
lgtm
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289333005/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289333005/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/c5bab94fdde60b2bc9ed93822f35c5ce50202d26 Cr-Commit-Position: refs/heads/master@{#356371}
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:440001) has been created in https://codereview.chromium.org/1405993006/ by gab@chromium.org. The reason for reverting is: Cause of http://crbug.com/548965 and http://crbug.com/548964, need to figure out what's going on there before this goes to a larger population. Only Canary was affected for now and users can recover through uninstall/reinstall (or even potentially just reboot if it's a Windows shell caching issue)..
Message was sent while issue was closed.
On 2015/10/30 02:23:45, gab wrote: > A revert of this CL (patchset #23 id:440001) has been created in > https://codereview.chromium.org/1405993006/ by https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org. > > The reason for reverting is: Cause of http://crbug.com/548965 and > http://crbug.com/548964, need to figure out what's going on there before this > goes to a larger population. > > Only Canary was affected for now and users can recover through > uninstall/reinstall (or even potentially just reboot if it's a Windows shell > caching issue).. Sorry about that, tried to do a mini-revert but was getting too complex (see discussion on https://codereview.chromium.org/1410973006/). I suggest re-opening this CL (from the Edit Issue menu) and continuing with follow-up patch sets (or if you decide to start from a new CL, make sure to make patch set 1 an identical copy of this one so that we only have to review the diff). Thanks, Gab
Message was sent while issue was closed.
Description was changed from ========== Change shortcut install location to non-subdir. BUG=169669 Committed: https://crrev.com/c5bab94fdde60b2bc9ed93822f35c5ce50202d26 Cr-Commit-Position: refs/heads/master@{#356371} ========== to ========== Change shortcut install location to non-subdir. BUG=169669 Committed: https://crrev.com/c5bab94fdde60b2bc9ed93822f35c5ce50202d26 Cr-Commit-Position: refs/heads/master@{#356371} ==========
Preparing to re-submit now that cl/1438793002 has paved the way for restart. Part of that CL is reverted here since it's only necessary when switching from the previous version to this one.
bcwhite@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in chrome_browser_main_win.cc The only change is from SHORTCUT_LOCATION_START_MENU_CHROME_DIR to SHORTCUT_LOCATION_START_MENU_CHROME_DIR_DEPRECATED plus an indent fix from "git cl format".
re-lgtm, thanks
lgtm
jhawkins@chromium.org changed reviewers: - jhawkins@chromium.org
still lgtm
jochen@chromium.org changed reviewers: + jhawkins@chromium.org
lgtm
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jhawkins@chromium.org Link to the patchset: https://codereview.chromium.org/1289333005/#ps500001 (title: "revert part of cl/1438793002 that is no longer needed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289333005/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289333005/500001
Message was sent while issue was closed.
Committed patchset #25 (id:500001)
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/f440aa1a1e31c13ffc981612f50f330e944e2bb4 Cr-Commit-Position: refs/heads/master@{#360867} |
