Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1780693002:
Fix the path of Chrome shortcuts from the installer. (Closed)
DescriptionFix the path of Chrome shortcuts from the installer.
With this CL, the installer looks for shortcuts with a target path that
starts with the install directory and ends with *chrome.exe. It updates
all these shortcuts to point to the correct Chrome executable.
This will fix shortcuts broken by https://codereview.chromium.org/1666363002/,
which have a path that looks like this:
C:\Users\<user>\AppData\Local\Google\Chrome SxS\Temp\scoped_dir_<x>\new_chrome.exe
R=gab@chromium.org,grt@chromium.org
BUG=592040
Committed: https://crrev.com/ff323ec5588d6e61a4267ab32251cf2310c84533
Cr-Commit-Position: refs/heads/master@{#380405}
Patch Set 1 #
Total comments: 28
Patch Set 2 : CR #Patch Set 3 : CR #Patch Set 4 : self review #
Total comments: 6
Patch Set 5 : CR #Messages
Total messages: 11 (3 generated)
Gab or Greg, can you review this CL? Thanks.
lg, comments below. @grt: do you feel like deep recursing through "C:\Users\Gabriel\AppData\Roaming\Microsoft\Internet Explorer\Quick Launch\" is safe? Ref. discussion @ https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... Thanks! Gab https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:381: shortcut_operation == ShellUtil::SHELL_SHORTCUT_REPLACE_EXISTING) { I was initially going to say that these are redundant when touching the per-user shortcut, but I guess they're not quite redundant as they "REPLACE" instead of "UPDATE" -- so it's the update task that's redundant, but since the update spans more than just one shortcut we can't cut it either even when both operations are done at ShellUtil::CURRENT_USER level... So I guess having both (as you have it) is the simplest way to write this despite the minor redundancy. WDYT? https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:424: // Update the target path of existing shortcuts. s/shortcuts/per-user shortcuts/ https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:426: const base::FilePath target_path_prefix = target.DirName(); If I'm not mistaken, |target_path| here will be something like C:\Users\<user>\AppData\Local\Google\Chrome SxS\Application\chrome.exe so I think for the prefix you want .DirName().DirName() to be one-level below Application/ https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:426: const base::FilePath target_path_prefix = target.DirName(); How about: s/target_path_prefix/updated_prefix/ s/target_path_suffix/updated_suffix/ ? https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:429: UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_DESKTOP, shortcut_level, dist, s/shortcut_level/ShellUtil::CURRENT_USER/ here and below (but don't add "&& shortcut_level == ShellUtil::CURRENT_USER" to initial condition as fixing up per-user shortcuts on system-level installs has the advantage to also work for any user running the installer as admin for himself (instead of say an update running as SYSTEM which isn't a "user" in the shortcut world). https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:431: UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_START_MENU_ROOT, Don't think we need to touch the Start Menu shortcut -- it's always at install level and thus already updated above. Plus recursing through all shortcuts in Start Menu seems overkill (I guess it could fixup some Chrome Apps shortcut, but if that's a goal we should explicit list the Apps Shortcut folder here instead of SHORTCUT_LOCATION_START_MENU_ROOT). https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:435: // Check that the quick launch directory is a parent of the taskbar pins // The next update assumes that the quick launch directory is a parent of the taskbar pins directory, confirm that this is true. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:446: // This updates both quick launch and taskbar pins. // Update quick launch and taskbar pins. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... File chrome/installer/setup/update_shortcuts.cc (right): https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.cc:29: base::FileEnumerator shortcuts_enum(shortcut_path, true, // recursive Blind recursion here is potentially dangerous (i.e. anyone can hook a very deep file hierarchy in these folders -- for example I have machines with my Google Drive folder on the desktop...). I think I'm okay with recursing through QuickLaunch/TaskbarPins as those paths aren't meant to hold user folders, but this should be a parameter of this method instead of something we do for all of them. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.cc:33: base::string16 existing_args_unused; ResolveShortcut() supports null arguments, so no need for this unused var (in fact using it creates work that ResolveShortcut knows to avoid if it is null :-)). https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... File chrome/installer/setup/update_shortcuts.h (right): https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:19: // existing target path matching |old_target_path_prefix| and s/matching/starting with/ https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:20: // |old_target_path_suffix| in the location specified by |shortcut_location|, s/and/and ending with/ https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:21: // |shortcut_level| and |dist|. s/ and/, and/ https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:22: void UpdateShortcuts(const ShellUtil::ShortcutLocation shortcut_location, A more specific name would be nice (we already have too many Install/Create/UpdateShortcuts() methods :-)!). How about "UpdatePerUserShortcutsInLocation()"? (and then drop |shortcut_level| param and just hardcode ShellUtil::CURRENT_USER)
Done. Can you take another look? https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:381: shortcut_operation == ShellUtil::SHELL_SHORTCUT_REPLACE_EXISTING) { On 2016/03/09 20:10:44, gab wrote: > I was initially going to say that these are redundant when touching the per-user > shortcut, but I guess they're not quite redundant as they "REPLACE" instead of > "UPDATE" -- so it's the update task that's redundant, but since the update spans > more than just one shortcut we can't cut it either even when both operations are > done at ShellUtil::CURRENT_USER level... > > So I guess having both (as you have it) is the simplest way to write this > despite the minor redundancy. > > WDYT? They are redundant. This code should be refactored, but I wanted to get a fix that works as quickly as possible. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:424: // Update the target path of existing shortcuts. On 2016/03/09 20:10:44, gab wrote: > s/shortcuts/per-user shortcuts/ Done. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:426: const base::FilePath target_path_prefix = target.DirName(); On 2016/03/09 20:10:44, gab wrote: > How about: > > s/target_path_prefix/updated_prefix/ > s/target_path_suffix/updated_suffix/ > > ? Done. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:426: const base::FilePath target_path_prefix = target.DirName(); On 2016/03/09 20:10:44, gab wrote: > If I'm not mistaken, |target_path| here will be something like > C:\Users\<user>\AppData\Local\Google\Chrome SxS\Application\chrome.exe > so I think for the prefix you want .DirName().DirName() to be one-level below > Application/ Done. I didn't use the path from the bug report to test this code initially... I did test the updated CL with the path from the bug report. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:429: UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_DESKTOP, shortcut_level, dist, On 2016/03/09 20:10:44, gab wrote: > s/shortcut_level/ShellUtil::CURRENT_USER/ > > here and below (but don't add "&& shortcut_level == ShellUtil::CURRENT_USER" to > initial condition as fixing up per-user shortcuts on system-level installs has > the advantage to also work for any user running the installer as admin for > himself (instead of say an update running as SYSTEM which isn't a "user" in the > shortcut world). CURRENT_USER now hard-coded in UpdatePerUserShortcutsInLocation https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:431: UpdateShortcuts(ShellUtil::SHORTCUT_LOCATION_START_MENU_ROOT, On 2016/03/09 20:10:43, gab wrote: > Don't think we need to touch the Start Menu shortcut -- it's always at install > level and thus already updated above. Plus recursing through all shortcuts in > Start Menu seems overkill (I guess it could fixup some Chrome Apps shortcut, but > if that's a goal we should explicit list the Apps Shortcut folder here instead > of SHORTCUT_LOCATION_START_MENU_ROOT). Done. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:435: // Check that the quick launch directory is a parent of the taskbar pins On 2016/03/09 20:10:44, gab wrote: > // The next update assumes that the quick launch directory is a parent of the > taskbar pins directory, confirm that this is true. I now have separate calls to UpdatePerUserShortcutsInLocation to update quick launch and taskbar pins. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/inst... chrome/installer/setup/install.cc:446: // This updates both quick launch and taskbar pins. On 2016/03/09 20:10:43, gab wrote: > // Update quick launch and taskbar pins. This comment is no longer applicable. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... File chrome/installer/setup/update_shortcuts.cc (right): https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.cc:29: base::FileEnumerator shortcuts_enum(shortcut_path, true, // recursive On 2016/03/09 20:10:44, gab wrote: > Blind recursion here is potentially dangerous (i.e. anyone can hook a very deep > file hierarchy in these folders -- for example I have machines with my Google > Drive folder on the desktop...). > > I think I'm okay with recursing through QuickLaunch/TaskbarPins as those paths > aren't meant to hold user folders, but this should be a parameter of this method > instead of something we do for all of them. ok, I now use recursion only for taskbar pins. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.cc:33: base::string16 existing_args_unused; On 2016/03/09 20:10:44, gab wrote: > ResolveShortcut() supports null arguments, so no need for this unused var (in > fact using it creates work that ResolveShortcut knows to avoid if it is null > :-)). Done. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... File chrome/installer/setup/update_shortcuts.h (right): https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:19: // existing target path matching |old_target_path_prefix| and On 2016/03/09 20:10:44, gab wrote: > s/matching/starting with/ Done. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:20: // |old_target_path_suffix| in the location specified by |shortcut_location|, On 2016/03/09 20:10:44, gab wrote: > s/and/and ending with/ Done. https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:21: // |shortcut_level| and |dist|. On 2016/03/09 20:10:44, gab wrote: > s/ and/, and/ no longer applies https://codereview.chromium.org/1780693002/diff/1/chrome/installer/setup/upda... chrome/installer/setup/update_shortcuts.h:22: void UpdateShortcuts(const ShellUtil::ShortcutLocation shortcut_location, On 2016/03/09 20:10:44, gab wrote: > A more specific name would be nice (we already have too many > Install/Create/UpdateShortcuts() methods :-)!). > > How about "UpdatePerUserShortcutsInLocation()"? > > (and then drop |shortcut_level| param and just hardcode ShellUtil::CURRENT_USER) Done.
lgtm w/ move to anonymous method. Thanks! Gab https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/update_per_user_shortcuts_in_location.cc (right): https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... chrome/installer/setup/update_per_user_shortcuts_in_location.cc:7: #include "base/command_line.h" Not required? https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... chrome/installer/setup/update_per_user_shortcuts_in_location.cc:14: #include "chrome/common/chrome_switches.h" Which switch is used? I don't see anything requiring this include. https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/update_per_user_shortcuts_in_location.h (right): https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... chrome/installer/setup/update_per_user_shortcuts_in_location.h:22: void UpdatePerUserShortcutsInLocation( Actually, given this is not used from anywhere else: how about making it an anonymous method in install.cc?
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1780693002/#ps80001 (title: "CR")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780693002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780693002/80001
https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/update_per_user_shortcuts_in_location.cc (right): https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... chrome/installer/setup/update_per_user_shortcuts_in_location.cc:7: #include "base/command_line.h" On 2016/03/10 06:01:03, gab wrote: > Not required? Done. https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... chrome/installer/setup/update_per_user_shortcuts_in_location.cc:14: #include "chrome/common/chrome_switches.h" On 2016/03/10 06:01:03, gab wrote: > Which switch is used? I don't see anything requiring this include. Done. https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... File chrome/installer/setup/update_per_user_shortcuts_in_location.h (right): https://codereview.chromium.org/1780693002/diff/60001/chrome/installer/setup/... chrome/installer/setup/update_per_user_shortcuts_in_location.h:22: void UpdatePerUserShortcutsInLocation( On 2016/03/10 06:01:03, gab wrote: > Actually, given this is not used from anywhere else: how about making it an > anonymous method in install.cc? Done.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix the path of Chrome shortcuts from the installer. With this CL, the installer looks for shortcuts with a target path that starts with the install directory and ends with *chrome.exe. It updates all these shortcuts to point to the correct Chrome executable. This will fix shortcuts broken by https://codereview.chromium.org/1666363002/, which have a path that looks like this: C:\Users\<user>\AppData\Local\Google\Chrome SxS\Temp\scoped_dir_<x>\new_chrome.exe R=gab@chromium.org,grt@chromium.org BUG=592040 ========== to ========== Fix the path of Chrome shortcuts from the installer. With this CL, the installer looks for shortcuts with a target path that starts with the install directory and ends with *chrome.exe. It updates all these shortcuts to point to the correct Chrome executable. This will fix shortcuts broken by https://codereview.chromium.org/1666363002/, which have a path that looks like this: C:\Users\<user>\AppData\Local\Google\Chrome SxS\Temp\scoped_dir_<x>\new_chrome.exe R=gab@chromium.org,grt@chromium.org BUG=592040 Committed: https://crrev.com/ff323ec5588d6e61a4267ab32251cf2310c84533 Cr-Commit-Position: refs/heads/master@{#380405} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ff323ec5588d6e61a4267ab32251cf2310c84533 Cr-Commit-Position: refs/heads/master@{#380405} |
