Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13864015:
Move app launcher and chrome apps shortcut strings into the installer (Closed)
|
Created:
7 years, 8 months ago by calamity Modified:
7 years ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@migrate_app_id_fix Visibility:
Public. |
DescriptionWe want the Applications Menu shortcut paths to be managed by shell_util.cc as
this will allow the uninstaller to clean up shortcuts. BrowserDistribution now
provides the name of a distribution-specific Chrome app subfolder, and
ShellUtil's utility functions can now create, update, and remove shortcuts
therein.
This CL introduces some structural changes:
-ShellIntegration::ShortcutLocations now uses an enum to specify the
Applications Menu location rather than a string. The enum consists of the root
of the applications menu, the "Google Chrome" subdirectory and the "Chrome
Apps" subdirectory.
-ShellUtil::ShortcutLocation has the equivalent locations which are implemented
by ShellUtil::GetShortcutPath().
-BrowserDistribution::GetStartMenuShortcutSubfolder() now
supports SUBFOLDER_APPS.
This CL can be supplementally tested by running setup_unittests.
This began a rework of https://codereview.chromium.org/13940006/ for gab@'s
post-commit comments.
BUG=233434, 238895
TEST=setup_unittests.exe, installer_util_unittests.exe
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238648
Patch Set 1 #
Total comments: 12
Patch Set 2 : rebase #Patch Set 3 : rework #
Total comments: 22
Patch Set 4 : rework #
Total comments: 2
Patch Set 5 : rebase, move app launcher and chrome app strings into the installer #
Total comments: 51
Patch Set 6 : rebase #Patch Set 7 : rework #
Total comments: 9
Patch Set 8 : rebase #Patch Set 9 : address comments #Patch Set 10 : make necessary linux changes #
Total comments: 3
Patch Set 11 : rebase #Patch Set 12 : fix for mac #Patch Set 13 : fix linux unit tests #
Total comments: 15
Patch Set 14 : rework #Patch Set 15 : rbrebase #Patch Set 16 : change in_application_menu* into and enum #Patch Set 17 : fix linux #
Total comments: 57
Patch Set 18 : rebase #Patch Set 19 : Win rework #Patch Set 20 : add test #Patch Set 21 : add #ifdef for linux only property #
Total comments: 10
Patch Set 22 : fix nits #
Total comments: 14
Patch Set 23 : fix windows nits #
Total comments: 6
Patch Set 24 : rebase #Patch Set 25 : fix linux nits #
Total comments: 2
Patch Set 26 : fix nits, change comment #Patch Set 27 : rebase #Patch Set 28 : fix win compile, change LOG(INFO)s to LOG(WARNING)s #Patch Set 29 : fix win compile, change LOG(INFO)s to VLOG(1) #Messages
Total messages: 62 (0 generated)
Comments below, on second-thought I think that the user-specific suffix is not required for the app use-case, it was mostly created to distinguish between user-level and system-level installs in the registry on Win8 and shouldn't affect apps which only use user-level shortcuts and don't register their appids in the registry. Sorry for the confusion :(. Cheers! Gab https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:101: app_name.append(suffix); The reason for Chrome that the suffix is attached to the app_name (Chrome) as a single component is legacy. The suffix used to be much less restricted (i.e. before I refactored everything to make it respect the rules for appid, progid, etc. as violations of those rules were breaking Chrome on Windows 8); Much of the code takes the suffix as a string and appends it directly to the {appid, app_name, progid}. The refactoring was already pretty big at the time and I didn't want to tweak all of these methods further. I feel there is another reason (perhaps simplicity to support the old-style suffix, see GetOldUserSpecificRegistrySuffix() if you're curious). This could probably be fixed in its own CL, but I'm not convinced it's worth it. The result of BuilAppModelId for a given set of components is always the same; if the suffix is truncated it hinders it's uniqueness, but it's really not that bad given it's already extremely unique (i.e. 32^26 possibilities..!). https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/app_list/app_list_controller_win.cc:163: ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL; Actually, if this is called by Chrome running as the user, it won't be able to create system-level shortcuts :(, I always prefer user-level shortcut anyways (and tried really hard to get rid of all-users (system-level) shortcuts, but couldn't quite get there because of having to support legacy installs...), I somehow hadn't fully grasped what was going on when I made the initial comment :(. That being said, if you are fine with only ever having per-user shortcuts, I think you can forget about the user-specific-suffix (it's only required if there is a possibility that a system-level and user-level install will co-exist and is mostly important for the second usage of an appid (i.e. launching the right Chrome in Win8), not so much for the taskbar grouping usage of the appid since system-level and user-level Chrome will have the same runtime appid for apps anyways... The only way you will ever end up with system-level shortcuts is if the installer creates them (since it runs as admin when installing a system-level Chrome) and this doesn't seem to be the plan. Sorry I lead you down this erroneous path :(. In short I think it's safe to not use the suffix and hardcode ShellUtil::CURRENT_USER when calling ShellUtil::GetShortcutPath(). https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/app_list/app_list_controller_win.cc:207: base::win::TaskbarPinShortcutLink(shortcut_file.value().c_str()); Only pin if the creation above was successful. https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... File chrome/browser/web_applications/web_app.h (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:106: const ShellUtil::ShellChange install_level); I don't think ShellUtil:: is available on non-OS_WIN so this probably wouldn't compile on other platforms, but as suggested above I think it's fine to hardcode ShellUtil::CURRENT_USER down below in the call to ShellUtil::GetShortcutPath(). https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:333: ShellUtil::SHORTCUT_LOCATION_START_MENU, SHORTCUT_LOCATION_START_MENU_CHROME_DIR sounds good to me. Which shortcuts are not going in the Chrome dir? Only Chrome and Applist are in the Google Chrome folder and all apps are outside of it? https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:337: // For Win7, in_quick_launch_bar means pinning to taskbar. This needs to I don't think that this comment is correct. Quick Launch is NOT the taskbar, they are different folders and both exist on Win7. Unless what is meant here is that the option in the UI is changed to say "taskbar", but it is still |in_quick_launch_bar| that gets set to true if the user sets that for web app shortcuts??
Let me know if there's anything else. =) https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/shell_i... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/shell_i... chrome/browser/shell_integration_win.cc:101: app_name.append(suffix); On 2013/04/26 15:21:03, gab wrote: > The reason for Chrome that the suffix is attached to the app_name (Chrome) as a > single component is legacy. The suffix used to be much less restricted (i.e. > before I refactored everything to make it respect the rules for appid, progid, > etc. as violations of those rules were breaking Chrome on Windows 8); Much of > the code takes the suffix as a string and appends it directly to the {appid, > app_name, progid}. The refactoring was already pretty big at the time and I > didn't want to tweak all of these methods further. I feel there is another > reason (perhaps simplicity to support the old-style suffix, see > GetOldUserSpecificRegistrySuffix() if you're curious). This could probably be > fixed in its own CL, but I'm not convinced it's worth it. > > The result of BuilAppModelId for a given set of components is always the same; > if the suffix is truncated it hinders it's uniqueness, but it's really not that > bad given it's already extremely unique (i.e. 32^26 possibilities..!). Ah ok, cool. Part of the reason I was asking was that I thought that losing a '.' character would affect things. Either way, this has been removed so it's moot. https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/ui/view... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/app_list/app_list_controller_win.cc:163: ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL; On 2013/04/26 15:21:03, gab wrote: > Actually, if this is called by Chrome running as the user, it won't be able to > create system-level shortcuts :(, I always prefer user-level shortcut anyways > (and tried really hard to get rid of all-users (system-level) shortcuts, but > couldn't quite get there because of having to support legacy installs...), I > somehow hadn't fully grasped what was going on when I made the initial comment > :(. > > That being said, if you are fine with only ever having per-user shortcuts, I > think you can forget about the user-specific-suffix (it's only required if there > is a possibility that a system-level and user-level install will co-exist and is > mostly important for the second usage of an appid (i.e. launching the right > Chrome in Win8), not so much for the taskbar grouping usage of the appid since > system-level and user-level Chrome will have the same runtime appid for apps > anyways... > > The only way you will ever end up with system-level shortcuts is if the > installer creates them (since it runs as admin when installing a system-level > Chrome) and this doesn't seem to be the plan. > > Sorry I lead you down this erroneous path :(. > > In short I think it's safe to not use the suffix and hardcode > ShellUtil::CURRENT_USER when calling ShellUtil::GetShortcutPath(). Done. Thanks for explaining this. https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/ui/view... chrome/browser/ui/views/app_list/app_list_controller_win.cc:207: base::win::TaskbarPinShortcutLink(shortcut_file.value().c_str()); On 2013/04/26 15:21:03, gab wrote: > Only pin if the creation above was successful. Done. https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... File chrome/browser/web_applications/web_app.h (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:106: const ShellUtil::ShellChange install_level); On 2013/04/26 15:21:03, gab wrote: > I don't think ShellUtil:: is available on non-OS_WIN so this probably wouldn't > compile on other platforms, but as suggested above I think it's fine to hardcode > ShellUtil::CURRENT_USER down below in the call to ShellUtil::GetShortcutPath(). Done. https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... File chrome/browser/web_applications/web_app_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:333: ShellUtil::SHORTCUT_LOCATION_START_MENU, On 2013/04/26 15:21:03, gab wrote: > SHORTCUT_LOCATION_START_MENU_CHROME_DIR sounds good to me. > > Which shortcuts are not going in the Chrome dir? Only Chrome and Applist are in > the Google Chrome folder and all apps are outside of it? mgiuca@ is making chrome app start menu shortcuts get created under a top-level folder tentatively named 'Chrome Apps'. In fact, there's probably no reason to provide full access to the start menu. I'll just add a TODO for him to add another value to the ShellUtil::ShortcutLocations enum. He's also brought to my attention that having an arbitrary subdirectory breaks the shortcut cleanup paradigm of "Search everywhere we could have made shortcuts". Do you think it's still worth changing the enum value name? https://chromiumcodereview.appspot.com/13864015/diff/1/chrome/browser/web_app... chrome/browser/web_applications/web_app_win.cc:337: // For Win7, in_quick_launch_bar means pinning to taskbar. This needs to On 2013/04/26 15:21:03, gab wrote: > I don't think that this comment is correct. > Quick Launch is NOT the taskbar, they are different folders and both exist on > Win7. Unless what is meant here is that the option in the UI is changed to say > "taskbar", but it is still |in_quick_launch_bar| that gets set to true if the > user sets that for web app shortcuts?? Yes, in_quick_launch_bar is used as "pin to taskbar" in ShellIntegration::ShortcutLocations for Win7+. It is ignored by non-windows platforms.
Please add BUG=233434 to CL description. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... chrome/browser/shell_integration_win.cc:86: nit: Remove extra empty line. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... File chrome/browser/shell_integration_win_unittest.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... chrome/browser/shell_integration_win_unittest.cc:340: base_app_id.append(suffix); You can remove this code. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (left): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:66: #include "chrome/installer/util/install_util.h" I don't see what in this diff justifies this change in the includes anymore. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:174: base::FilePath chrome_exe; This code can comeback here (i.e. let's keep this diff minimal, no reason to move it anymore). And keep the return statement you removed when moving it. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:201: AddExtension(installer::kLnkExt); nit: indent https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:203: shortcut_properties, nit: indent https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:205: base::win::TaskbarPinShortcutLink(shortcut_file.value().c_str()); Add {} around if content since condition spans more than one line. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/web_... File chrome/browser/web_applications/web_app_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/web_... chrome/browser/web_applications/web_app_win.cc:324: ShellUtil::SHORTCUT_LOCATION_START_MENU, This will already return the StartMenu/Google Chrome path, no need to add the subdir again. The whole subdir logic can be removed from this method and from ShellIntegration::ShortcutLocations I think. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/installer/ut... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/installer/ut... chrome/installer/util/shell_util.h:47: // TODO(mgiuca): add SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR here. FYI this will conflict with https://codereview.chromium.org/14287008/ https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/installer/ut... chrome/installer/util/shell_util.h:47: // TODO(mgiuca): add SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR here. I don't think a TODO is required here since none of this Chrome Apps folder logic has been implemented yet and this might not end up being where it is implemented...
https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... chrome/installer/util/shell_util.h:47: // TODO(mgiuca): add SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR here. On 2013/04/29 21:34:44, gab wrote: > I don't think a TODO is required here since none of this Chrome Apps folder > logic has been implemented yet and this might not end up being where it is > implemented... Seems indeed that https://codereview.chromium.org/14533004/ is going in another direction (although it might be wrong and end up using ShellUtil::), looks like you and Matt should have a chat :)!
https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... chrome/installer/util/shell_util.h:47: // TODO(mgiuca): add SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR here. On 2013/04/29 21:45:28, gab wrote: > On 2013/04/29 21:34:44, gab wrote: > > I don't think a TODO is required here since none of this Chrome Apps folder > > logic has been implemented yet and this might not end up being where it is > > implemented... > > Seems indeed that https://codereview.chromium.org/14533004/ is going in another > direction (although it might be wrong and end up using ShellUtil::), looks like > you and Matt should have a chat :)! Hi Gab, Yes, I'm aware of this CL (it was my suggestion that Chris add me as a TODO here). My '004 CL is required functionality for M28 so I am trying to get it working as quickly as possible without blocking on this refactor (which might not make it into M28). Currently the Chrome Apps shortcut creation code is unrelated to shell_util, so it will be some degree of work to fix it. I'm happy to remove the TODO and we'll just keep this refactor on the radar.
https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... File chrome/browser/shell_integration_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... chrome/browser/shell_integration_win.cc:86: On 2013/04/29 21:34:44, gab wrote: > nit: Remove extra empty line. Done. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... File chrome/browser/shell_integration_win_unittest.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/shel... chrome/browser/shell_integration_win_unittest.cc:340: base_app_id.append(suffix); On 2013/04/29 21:34:44, gab wrote: > You can remove this code. Done. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (left): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:66: #include "chrome/installer/util/install_util.h" On 2013/04/29 21:34:44, gab wrote: > I don't see what in this diff justifies this change in the includes anymore. Done. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:174: base::FilePath chrome_exe; On 2013/04/29 21:34:44, gab wrote: > This code can comeback here (i.e. let's keep this diff minimal, no reason to > move it anymore). > > And keep the return statement you removed when moving it. Done. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:201: AddExtension(installer::kLnkExt); On 2013/04/29 21:34:44, gab wrote: > nit: indent Done. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:203: shortcut_properties, On 2013/04/29 21:34:44, gab wrote: > nit: indent Done. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/ui/v... chrome/browser/ui/views/app_list/app_list_controller_win.cc:205: base::win::TaskbarPinShortcutLink(shortcut_file.value().c_str()); On 2013/04/29 21:34:44, gab wrote: > Add {} around if content since condition spans more than one line. Done. https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/web_... File chrome/browser/web_applications/web_app_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/7002/chrome/browser/web_... chrome/browser/web_applications/web_app_win.cc:324: ShellUtil::SHORTCUT_LOCATION_START_MENU, On 2013/04/29 21:34:44, gab wrote: > This will already return the StartMenu/Google Chrome path, no need to add the > subdir again. The whole subdir logic can be removed from this method and from > ShellIntegration::ShortcutLocations I think. Done.
Arg, do we have a clear design doc on what we want the result to be as far as shortcuts are concerned for AppsV2? Timezones are such that I only get to give you feedback once a day and every day I learn something different which makes me doubt my suggestions from the previous day... Perhaps it's best to avoid ShellUtil for now and to consider re-using it later when things have settled a bit? Cheers, Gab https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/ap... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/ap... chrome/browser/ui/views/app_list/app_list_controller_win.cc:201: AddExtension(installer::kLnkExt); On 2013/04/30 08:11:28, calamity wrote: > On 2013/04/29 21:34:44, gab wrote: > > nit: indent > > Done. I meant that it was indented 6 spaces instead of 4, the new indent is incorrect. https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... chrome/installer/util/shell_util.h:47: // TODO(mgiuca): add SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR here. On 2013/04/30 01:14:09, Matt Giuca wrote: > On 2013/04/29 21:45:28, gab wrote: > > On 2013/04/29 21:34:44, gab wrote: > > > I don't think a TODO is required here since none of this Chrome Apps folder > > > logic has been implemented yet and this might not end up being where it is > > > implemented... > > > > Seems indeed that https://codereview.chromium.org/14533004/ is going in > another > > direction (although it might be wrong and end up using ShellUtil::), looks > like > > you and Matt should have a chat :)! > > Hi Gab, > > Yes, I'm aware of this CL (it was my suggestion that Chris add me as a TODO > here). My '004 CL is required functionality for M28 so I am trying to get it > working as quickly as possible without blocking on this refactor (which might > not make it into M28). Currently the Chrome Apps shortcut creation code is > unrelated to shell_util, so it will be some degree of work to fix it. > > I'm happy to remove the TODO and we'll just keep this refactor on the radar. I see, thanks for clarifying, I'd prefer not to have a TODO for now since I'm not yet convinced this belongs here anyways. https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/a... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:207: shortcut_properties, base::win::SHORTCUT_CREATE_ALWAYS)) { This indent is still incorrect, the rule is to either wrap params to the parenthesis or 4 spaces in (never a mix of both styles), e.g.: base::win::CreateOrUpdateShortcutLink( shortcut_file, shortcut_properties, base::win::SHORTCUT_CREATE_ALWAYS) https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applic... chrome/browser/web_applications/web_app_win.cc:322: ShellUtil::SHORTCUT_LOCATION_START_MENU Sorry for being inconsistent on this review, but I keep getting new information... seems https://codereview.chromium.org/14533004/ uses application_sub_menu_dir removed here... :(. Also I just realized that you don't really need the ShellUtil functionality provided by GetShortcutPath anymore... it handles user-level vs system-level (but we don't care about this anymore) and the subdir for the Start Menu, but it's not clear that we want this all the time now... Based on a comment on the other CL: "Ben says: Why are regular website applications being put into the "Chrome Apps" submenu? They should continue to be created at the top level. Hosted apps and V1 Packaged apps are fine to be in the "Chrome Apps" menu." Currently this code would result in all apps landing in the Google Chrome folder. It seems there is a need for some shortcuts in the regular start menu, some in the Google Chrome folder, some in the Chrome Apps folder... I'm not clear on what the spec is here, but it definitely seems like ShellUtil doesn't do what you need. Perhaps you want to use base_paths directly (as this used to...) and go back to using the sub_dir logic (but using BrowserDistribution to get the AppShortcutName() instead of getting the string explicitly from i10n::)? Otherwise the ShellUtil logic could be augmented, but I'm not convinced that logic belongs in ShellUtil.
On 2013/04/30 15:00:24, gab wrote: > Arg, do we have a clear design doc on what we want the result to be as far as > shortcuts are concerned for AppsV2? Timezones are such that I only get to give > you feedback once a day and every day I learn something different which makes me > doubt my suggestions from the previous day... > > Perhaps it's best to avoid ShellUtil for now and to consider re-using it later > when things have settled a bit? > > Cheers, > Gab > > https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/ap... > File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): > > https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/ap... > chrome/browser/ui/views/app_list/app_list_controller_win.cc:201: > AddExtension(installer::kLnkExt); > On 2013/04/30 08:11:28, calamity wrote: > > On 2013/04/29 21:34:44, gab wrote: > > > nit: indent > > > > Done. > > I meant that it was indented 6 spaces instead of 4, the new indent is incorrect. > > https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... > File chrome/installer/util/shell_util.h (right): > > https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shel... > chrome/installer/util/shell_util.h:47: // TODO(mgiuca): add > SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR here. > On 2013/04/30 01:14:09, Matt Giuca wrote: > > On 2013/04/29 21:45:28, gab wrote: > > > On 2013/04/29 21:34:44, gab wrote: > > > > I don't think a TODO is required here since none of this Chrome Apps > folder > > > > logic has been implemented yet and this might not end up being where it is > > > > implemented... > > > > > > Seems indeed that https://codereview.chromium.org/14533004/ is going in > > another > > > direction (although it might be wrong and end up using ShellUtil::), looks > > like > > > you and Matt should have a chat :)! > > > > Hi Gab, > > > > Yes, I'm aware of this CL (it was my suggestion that Chris add me as a TODO > > here). My '004 CL is required functionality for M28 so I am trying to get it > > working as quickly as possible without blocking on this refactor (which might > > not make it into M28). Currently the Chrome Apps shortcut creation code is > > unrelated to shell_util, so it will be some degree of work to fix it. > > > > I'm happy to remove the TODO and we'll just keep this refactor on the radar. > > I see, thanks for clarifying, I'd prefer not to have a TODO for now since I'm > not yet convinced this belongs here anyways. > > https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/a... > File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): > > https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/a... > chrome/browser/ui/views/app_list/app_list_controller_win.cc:207: > shortcut_properties, base::win::SHORTCUT_CREATE_ALWAYS)) { > This indent is still incorrect, the rule is to either wrap params to the > parenthesis or 4 spaces in (never a mix of both styles), e.g.: > base::win::CreateOrUpdateShortcutLink( > shortcut_file, shortcut_properties, > base::win::SHORTCUT_CREATE_ALWAYS) > > https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app_win.cc (right): > > https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applic... > chrome/browser/web_applications/web_app_win.cc:322: > ShellUtil::SHORTCUT_LOCATION_START_MENU > Sorry for being inconsistent on this review, but I keep getting new > information... seems https://codereview.chromium.org/14533004/ uses > application_sub_menu_dir removed here... :(. > > Also I just realized that you don't really need the ShellUtil functionality > provided by GetShortcutPath anymore... it handles user-level vs system-level > (but we don't care about this anymore) and the subdir for the Start Menu, but > it's not clear that we want this all the time now... > > Based on a comment on the other CL: "Ben says: Why are regular website > applications being put into the "Chrome Apps" submenu? They should continue to > be created at the top level. Hosted apps and V1 Packaged apps are fine to be in > the "Chrome Apps" menu." > > Currently this code would result in all apps landing in the Google Chrome > folder. It seems there is a need for some shortcuts in the regular start menu, > some in the Google Chrome folder, some in the Chrome Apps folder... I'm not > clear on what the spec is here, but it definitely seems like ShellUtil doesn't > do what you need. > > Perhaps you want to use base_paths directly (as this used to...) and go back to > using the sub_dir logic (but using BrowserDistribution to get the > AppShortcutName() instead of getting the string explicitly from i10n::)? > Otherwise the ShellUtil logic could be augmented, but I'm not convinced that > logic belongs in ShellUtil. Yeah, the time difference is making this harder. One of the reasons to try to use ShellUtil is that uninstall code uses it for shortcut cleanup and we'll have a more consistent base if we use it wherever we can? FYI, Matt and Ben sit right behind me and we've been talking about this a little. I think we should have 3 ShellUtil enum values for Start Menu/, Start Menu/Google Chrome and Start Menu/Chrome Apps. Currently, the logic in GetShortcutPath that always appends the app shortcut name to Start Menu/ breaks as Google Chrome App Launcher is now in Start Menu/Google Chrome (see https://crbug.com/236871). This was causing ShellUtil::RemoveShortcuts to try to delete the shortcut in Start Menu/Google Chrome App Launcher. GetShortcutPath will use: - BrowserDistribution::GetDistribution()->GetAppShortCutName() for Start Menu/Google Chrome - l10n_util::GetStringUTF16(IDS_APP_SHORTCUTS_SUBDIR_NAME) for Start Menu/Chrome Apps Making this change would involve refactoring Matt's change a bit, but I can do that in this CL I guess. What do you think?
Awesome, yes, had been thinking the same thing overnight. This will also make it easy to fix http://crbug.com/171645 as we will have a ShortcutLocation for the start menu itself :). Try to coordinate with https://chromiumcodereview.appspot.com/14287008/from huangs@which is making the ShellUtil API for shortcut deletion/migration much better :). Please update the CL description accordingly if this ends up being more than a simple rework :). Cheers, Gab On May 1, 2013 12:37 AM, <calamity@chromium.org> wrote: > On 2013/04/30 15:00:24, gab wrote: > >> Arg, do we have a clear design doc on what we want the result to be as >> far as >> shortcuts are concerned for AppsV2? Timezones are such that I only get to >> give >> you feedback once a day and every day I learn something different which >> makes >> > me > >> doubt my suggestions from the previous day... >> > > Perhaps it's best to avoid ShellUtil for now and to consider re-using it >> later >> when things have settled a bit? >> > > Cheers, >> Gab >> > > > https://codereview.chromium.**org/13864015/diff/7002/chrome/** > browser/ui/views/app_list/app_**list_controller_win.cc<https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/app_list/app_list_controller_win.cc> > >> File chrome/browser/ui/views/app_**list/app_list_controller_win.**cc >> (right): >> > > > https://codereview.chromium.**org/13864015/diff/7002/chrome/** > browser/ui/views/app_list/app_**list_controller_win.cc#**newcode201<https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode201> > >> chrome/browser/ui/views/app_**list/app_list_controller_win.**cc:201: >> AddExtension(installer::**kLnkExt); >> On 2013/04/30 08:11:28, calamity wrote: >> > On 2013/04/29 21:34:44, gab wrote: >> > > nit: indent >> > >> > Done. >> > > I meant that it was indented 6 spaces instead of 4, the new indent is >> > incorrect. > > > https://codereview.chromium.**org/13864015/diff/7002/chrome/** > installer/util/shell_util.h<https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shell_util.h> > >> File chrome/installer/util/shell_**util.h (right): >> > > > https://codereview.chromium.**org/13864015/diff/7002/chrome/** > installer/util/shell_util.h#**newcode47<https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shell_util.h#newcode47> > >> chrome/installer/util/shell_**util.h:47: // TODO(mgiuca): add >> SHORTCUT_LOCATION_START_MENU_**CHROME_APPS_DIR here. >> On 2013/04/30 01:14:09, Matt Giuca wrote: >> > On 2013/04/29 21:45:28, gab wrote: >> > > On 2013/04/29 21:34:44, gab wrote: >> > > > I don't think a TODO is required here since none of this Chrome Apps >> folder >> > > > logic has been implemented yet and this might not end up being >> where it >> > is > >> > > > implemented... >> > > >> > > Seems indeed that https://codereview.chromium.**org/14533004/<https://codereview.chromium.org/1... going in >> > another >> > > direction (although it might be wrong and end up using ShellUtil::), >> looks >> > like >> > > you and Matt should have a chat :)! >> > >> > Hi Gab, >> > >> > Yes, I'm aware of this CL (it was my suggestion that Chris add me as a >> TODO >> > here). My '004 CL is required functionality for M28 so I am trying to >> get it >> > working as quickly as possible without blocking on this refactor (which >> > might > >> > not make it into M28). Currently the Chrome Apps shortcut creation code >> is >> > unrelated to shell_util, so it will be some degree of work to fix it. >> > >> > I'm happy to remove the TODO and we'll just keep this refactor on the >> radar. >> > > I see, thanks for clarifying, I'd prefer not to have a TODO for now since >> I'm >> not yet convinced this belongs here anyways. >> > > > https://codereview.chromium.**org/13864015/diff/26002/** > chrome/browser/ui/views/app_**list/app_list_controller_win.**cc<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/app_list/app_list_controller_win.cc> > >> File chrome/browser/ui/views/app_**list/app_list_controller_win.**cc >> (right): >> > > > https://codereview.chromium.**org/13864015/diff/26002/** > chrome/browser/ui/views/app_**list/app_list_controller_win.**cc#newcode207<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode207> > >> chrome/browser/ui/views/app_**list/app_list_controller_win.**cc:207: >> shortcut_properties, base::win::SHORTCUT_CREATE_**ALWAYS)) { >> This indent is still incorrect, the rule is to either wrap params to the >> parenthesis or 4 spaces in (never a mix of both styles), e.g.: >> base::win::**CreateOrUpdateShortcutLink( >> shortcut_file, shortcut_properties, >> base::win::SHORTCUT_CREATE_**ALWAYS) >> > > > https://codereview.chromium.**org/13864015/diff/26002/** > chrome/browser/web_**applications/web_app_win.cc<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applications/web_app_win.cc> > >> File chrome/browser/web_**applications/web_app_win.cc (right): >> > > > https://codereview.chromium.**org/13864015/diff/26002/** > chrome/browser/web_**applications/web_app_win.cc#**newcode322<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applications/web_app_win.cc#newcode322> > >> chrome/browser/web_**applications/web_app_win.cc:**322: >> ShellUtil::SHORTCUT_LOCATION_**START_MENU >> Sorry for being inconsistent on this review, but I keep getting new >> information... seems https://codereview.chromium.**org/14533004/<https://codereview.chromium.org/1... >> application_sub_menu_dir removed here... :(. >> > > Also I just realized that you don't really need the ShellUtil >> functionality >> provided by GetShortcutPath anymore... it handles user-level vs >> system-level >> (but we don't care about this anymore) and the subdir for the Start Menu, >> but >> it's not clear that we want this all the time now... >> > > Based on a comment on the other CL: "Ben says: Why are regular website >> applications being put into the "Chrome Apps" submenu? They should >> continue to >> be created at the top level. Hosted apps and V1 Packaged apps are fine to >> be >> > in > >> the "Chrome Apps" menu." >> > > Currently this code would result in all apps landing in the Google Chrome >> folder. It seems there is a need for some shortcuts in the regular start >> menu, >> some in the Google Chrome folder, some in the Chrome Apps folder... I'm >> not >> clear on what the spec is here, but it definitely seems like ShellUtil >> doesn't >> do what you need. >> > > Perhaps you want to use base_paths directly (as this used to...) and go >> back >> > to > >> using the sub_dir logic (but using BrowserDistribution to get the >> AppShortcutName() instead of getting the string explicitly from i10n::)? >> Otherwise the ShellUtil logic could be augmented, but I'm not convinced >> that >> logic belongs in ShellUtil. >> > > Yeah, the time difference is making this harder. > > One of the reasons to try to use ShellUtil is that uninstall code uses it > for > shortcut cleanup and we'll have a more consistent base if we use it > wherever we > can? > > FYI, Matt and Ben sit right behind me and we've been talking about this a > little. I think we should have 3 ShellUtil enum values for Start Menu/, > Start > Menu/Google Chrome and Start Menu/Chrome Apps. > > Currently, the logic in GetShortcutPath that always appends the app > shortcut > name to Start Menu/ breaks as Google Chrome App Launcher is now in Start > Menu/Google Chrome (see https://crbug.com/236871). This was causing > ShellUtil::RemoveShortcuts to try to delete the shortcut in Start > Menu/Google > Chrome App Launcher. > > GetShortcutPath will use: > - BrowserDistribution::**GetDistribution()->**GetAppShortCutName() for > Start > Menu/Google Chrome > - l10n_util::GetStringUTF16(IDS_**APP_SHORTCUTS_SUBDIR_NAME) for Start > Menu/Chrome > Apps > > Making this change would involve refactoring Matt's change a bit, but I > can do > that in this CL I guess. > > What do you think? > > https://chromiumcodereview.**appspot.com/13864015/<https://chromiumcodereview... >
From chromium address this time... On May 1, 2013 9:17 AM, "Gabriel Charette" <gab@google.com> wrote: > Awesome, yes, had been thinking the same thing overnight. > > This will also make it easy to fix http://crbug.com/171645 as we will > have a ShortcutLocation for the start menu itself :). > > Try to coordinate with https://chromiumcodereview.appspot.com/14287008/from huangs@which is making the ShellUtil API for shortcut deletion/migration much > better :). > > Please update the CL description accordingly if this ends up being more > than a simple rework :). > > Cheers, > Gab > On May 1, 2013 12:37 AM, <calamity@chromium.org> wrote: > >> On 2013/04/30 15:00:24, gab wrote: >> >>> Arg, do we have a clear design doc on what we want the result to be as >>> far as >>> shortcuts are concerned for AppsV2? Timezones are such that I only get >>> to give >>> you feedback once a day and every day I learn something different which >>> makes >>> >> me >> >>> doubt my suggestions from the previous day... >>> >> >> Perhaps it's best to avoid ShellUtil for now and to consider re-using it >>> later >>> when things have settled a bit? >>> >> >> Cheers, >>> Gab >>> >> >> >> https://codereview.chromium.**org/13864015/diff/7002/chrome/** >> browser/ui/views/app_list/app_**list_controller_win.cc<https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/app_list/app_list_controller_win.cc> >> >>> File chrome/browser/ui/views/app_**list/app_list_controller_win.**cc >>> (right): >>> >> >> >> https://codereview.chromium.**org/13864015/diff/7002/chrome/** >> browser/ui/views/app_list/app_**list_controller_win.cc#**newcode201<https://codereview.chromium.org/13864015/diff/7002/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode201> >> >>> chrome/browser/ui/views/app_**list/app_list_controller_win.**cc:201: >>> AddExtension(installer::**kLnkExt); >>> On 2013/04/30 08:11:28, calamity wrote: >>> > On 2013/04/29 21:34:44, gab wrote: >>> > > nit: indent >>> > >>> > Done. >>> >> >> I meant that it was indented 6 spaces instead of 4, the new indent is >>> >> incorrect. >> >> >> https://codereview.chromium.**org/13864015/diff/7002/chrome/** >> installer/util/shell_util.h<https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shell_util.h> >> >>> File chrome/installer/util/shell_**util.h (right): >>> >> >> >> https://codereview.chromium.**org/13864015/diff/7002/chrome/** >> installer/util/shell_util.h#**newcode47<https://codereview.chromium.org/13864015/diff/7002/chrome/installer/util/shell_util.h#newcode47> >> >>> chrome/installer/util/shell_**util.h:47: // TODO(mgiuca): add >>> SHORTCUT_LOCATION_START_MENU_**CHROME_APPS_DIR here. >>> On 2013/04/30 01:14:09, Matt Giuca wrote: >>> > On 2013/04/29 21:45:28, gab wrote: >>> > > On 2013/04/29 21:34:44, gab wrote: >>> > > > I don't think a TODO is required here since none of this Chrome >>> Apps >>> folder >>> > > > logic has been implemented yet and this might not end up being >>> where it >>> >> is >> >>> > > > implemented... >>> > > >>> > > Seems indeed that https://codereview.chromium.**org/14533004/<https://codereview.chromium.org/1... going in >>> > another >>> > > direction (although it might be wrong and end up using ShellUtil::), >>> looks >>> > like >>> > > you and Matt should have a chat :)! >>> > >>> > Hi Gab, >>> > >>> > Yes, I'm aware of this CL (it was my suggestion that Chris add me as a >>> TODO >>> > here). My '004 CL is required functionality for M28 so I am trying to >>> get it >>> > working as quickly as possible without blocking on this refactor (which >>> >> might >> >>> > not make it into M28). Currently the Chrome Apps shortcut creation >>> code is >>> > unrelated to shell_util, so it will be some degree of work to fix it. >>> > >>> > I'm happy to remove the TODO and we'll just keep this refactor on the >>> radar. >>> >> >> I see, thanks for clarifying, I'd prefer not to have a TODO for now >>> since I'm >>> not yet convinced this belongs here anyways. >>> >> >> >> https://codereview.chromium.**org/13864015/diff/26002/** >> chrome/browser/ui/views/app_**list/app_list_controller_win.**cc<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/app_list/app_list_controller_win.cc> >> >>> File chrome/browser/ui/views/app_**list/app_list_controller_win.**cc >>> (right): >>> >> >> >> https://codereview.chromium.**org/13864015/diff/26002/** >> chrome/browser/ui/views/app_**list/app_list_controller_win.** >> cc#newcode207<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/ui/views/app_list/app_list_controller_win.cc#newcode207> >> >>> chrome/browser/ui/views/app_**list/app_list_controller_win.**cc:207: >>> shortcut_properties, base::win::SHORTCUT_CREATE_**ALWAYS)) { >>> This indent is still incorrect, the rule is to either wrap params to the >>> parenthesis or 4 spaces in (never a mix of both styles), e.g.: >>> base::win::**CreateOrUpdateShortcutLink( >>> shortcut_file, shortcut_properties, >>> base::win::SHORTCUT_CREATE_**ALWAYS) >>> >> >> >> https://codereview.chromium.**org/13864015/diff/26002/** >> chrome/browser/web_**applications/web_app_win.cc<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applications/web_app_win.cc> >> >>> File chrome/browser/web_**applications/web_app_win.cc (right): >>> >> >> >> https://codereview.chromium.**org/13864015/diff/26002/** >> chrome/browser/web_**applications/web_app_win.cc#**newcode322<https://codereview.chromium.org/13864015/diff/26002/chrome/browser/web_applications/web_app_win.cc#newcode322> >> >>> chrome/browser/web_**applications/web_app_win.cc:**322: >>> ShellUtil::SHORTCUT_LOCATION_**START_MENU >>> Sorry for being inconsistent on this review, but I keep getting new >>> information... seems https://codereview.chromium.**org/14533004/<https://codereview.chromium.org/1... >>> application_sub_menu_dir removed here... :(. >>> >> >> Also I just realized that you don't really need the ShellUtil >>> functionality >>> provided by GetShortcutPath anymore... it handles user-level vs >>> system-level >>> (but we don't care about this anymore) and the subdir for the Start >>> Menu, but >>> it's not clear that we want this all the time now... >>> >> >> Based on a comment on the other CL: "Ben says: Why are regular website >>> applications being put into the "Chrome Apps" submenu? They should >>> continue to >>> be created at the top level. Hosted apps and V1 Packaged apps are fine >>> to be >>> >> in >> >>> the "Chrome Apps" menu." >>> >> >> Currently this code would result in all apps landing in the Google Chrome >>> folder. It seems there is a need for some shortcuts in the regular start >>> menu, >>> some in the Google Chrome folder, some in the Chrome Apps folder... I'm >>> not >>> clear on what the spec is here, but it definitely seems like ShellUtil >>> doesn't >>> do what you need. >>> >> >> Perhaps you want to use base_paths directly (as this used to...) and go >>> back >>> >> to >> >>> using the sub_dir logic (but using BrowserDistribution to get the >>> AppShortcutName() instead of getting the string explicitly from i10n::)? >>> Otherwise the ShellUtil logic could be augmented, but I'm not convinced >>> that >>> logic belongs in ShellUtil. >>> >> >> Yeah, the time difference is making this harder. >> >> One of the reasons to try to use ShellUtil is that uninstall code uses it >> for >> shortcut cleanup and we'll have a more consistent base if we use it >> wherever we >> can? >> >> FYI, Matt and Ben sit right behind me and we've been talking about this a >> little. I think we should have 3 ShellUtil enum values for Start Menu/, >> Start >> Menu/Google Chrome and Start Menu/Chrome Apps. >> >> Currently, the logic in GetShortcutPath that always appends the app >> shortcut >> name to Start Menu/ breaks as Google Chrome App Launcher is now in Start >> Menu/Google Chrome (see https://crbug.com/236871). This was causing >> ShellUtil::RemoveShortcuts to try to delete the shortcut in Start >> Menu/Google >> Chrome App Launcher. >> >> GetShortcutPath will use: >> - BrowserDistribution::**GetDistribution()->**GetAppShortCutName() for >> Start >> Menu/Google Chrome >> - l10n_util::GetStringUTF16(IDS_**APP_SHORTCUTS_SUBDIR_NAME) for Start >> Menu/Chrome >> Apps >> >> Making this change would involve refactoring Matt's change a bit, but I >> can do >> that in this CL I guess. >> >> What do you think? >> >> https://chromiumcodereview.**appspot.com/13864015/<https://chromiumcodereview... >> >
Hey, We have just run into another problem here. Am I correct in thinking that BrowserDistribution should be responsible for all distribution dependent strings? If so, we'll need to modify BrowserDistribution to provide the names Chrome Apps and Chrome App Launcher because we want different names per distribution for the shortcuts and shortcut folders, analogous to BrowserDistribution::GetAppShortCutName which provides the Google Chrome name. Some issues with this include all the weird distributions that don't deal with the app launcher like chrome frame and having to deal with those installer-only strings in installer_util_strings.h. (Is there a specific reason it can't just use the grd strings?) Any thoughts?
I'm not 100% sure what the separation of concern is regarding strings. I think installer_util_strings is statically linked into setup.exe (and thus we don't want all Chrome strings and use an independent target), but I'm not sure at all. Chrome Apps and Chrome App Launcher will be used by chrome.exe (although they might also need to be used by setup.exe for uninstalling). grt@ might have better insights as to where to put those strings (Greg these are the strings for the new Start Menu folder for "Chrome Apps" and for the "Chrome App Launcher" shortcut (to go in the Google Chrome folder)). Cheers, Gab
On 2013/05/02 14:28:03, gab wrote: > I'm not 100% sure what the separation of concern is regarding strings. > > I think installer_util_strings is statically linked into setup.exe (and thus we > don't want all Chrome strings and use an independent target), but I'm not sure > at all. Right. The strings needed by the installer are put into resources in the binary itself rather than pak files. http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/prebuil... is responsible for picking out the strings needed by the installer. If the per-distribution nature of the strings you're thinking of is limited to Google Chrome vs. Chromium, then perhaps you can get by by adding the correct strings to app/chromium_strings.grd and app/google_chrome_strings.grd. create_string_rc.py pulls from one or the other based on the brand at build-time.
On 2013/05/03 15:12:37, grt wrote: > On 2013/05/02 14:28:03, gab wrote: > > I'm not 100% sure what the separation of concern is regarding strings. > > > > I think installer_util_strings is statically linked into setup.exe (and thus > we > > don't want all Chrome strings and use an independent target), but I'm not sure > > at all. > > Right. The strings needed by the installer are put into resources in the binary > itself rather than pak files. > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/prebuil... > is responsible for picking out the strings needed by the installer. > > If the per-distribution nature of the strings you're thinking of is limited to > Google Chrome vs. Chromium, then perhaps you can get by by adding the correct > strings to app/chromium_strings.grd and app/google_chrome_strings.grd. > create_string_rc.py pulls from one or the other based on the brand at > build-time. I think we will want a separate string for Canary as well, so BrowserDistribution will definitely be useful..
+huangs Hello all, This change implements the changes previously suggested. The salient points are: -Add separate enums for Start Menu, Start Menu/Google Chrome and Start Menu/Chrome Apps in shell_util.cc -Add GetAppListShortcutName and GetAppShortcutsSubdirName to BrowserDistribution and all its subclasses The rest is changing other client code to deal with these two changes.
Looks like we're going to step on each other's toes a bit! Please see (especially shell_util.cc) https://chromiumcodereview.appspot.com/14031025/ Main areas of contention: - I added SHORTCUT_LOCATION_START_MENU_ROOT -- but this is for completeness and testing; I'm happy undo my changes if I merge after you. - Our approach toward BrowserDistribution change are fundamentally different. I rely on ChromeAppHostDistribution being around (always added when Chrome is installed), which is used to specify shortcut location, icon exe, icon index. This allows App Launcher shortcut to be created by the installer. To discern Chromium / Chrome / SxS I used localized ugliness, thus reducing # file changes. For app shortcuts, I envision this to be added in shell_util.cc, and not dist-dependent. Also, I use ChromeAppHostDistribution's GetAppShortcutName() instead of giving GetAppListShortcutName() to everyone. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... File chrome/browser/shell_integration.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... chrome/browser/shell_integration.cc:42: in_applications_menu(false), in_application_menu_root https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... chrome/browser/shell_integration.h:116: bool in_applications_menu; in_application_menu_root https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/s... chrome/installer/setup/uninstall.cc:356: if (!ShellUtil::RemoveShortcuts( How about SHORTCUT_LOCATION_START_MENU_CHROME_DIR and SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR? https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.h:327: // SHORTCUT_LOCATION_START_MENU_CHROME_DIR or Add "," back; I think Oxford comma improves readability for code. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.h:543: Delete extra line.
On 2013/05/15 02:33:46, huangs1 wrote: > Looks like we're going to step on each other's toes a bit! Please see > (especially shell_util.cc) > > https://chromiumcodereview.appspot.com/14031025/ > > Main areas of contention: > > - I added SHORTCUT_LOCATION_START_MENU_ROOT -- but this is for completeness and > testing; I'm happy undo my changes if I merge after you. > > - Our approach toward BrowserDistribution change are fundamentally different. I > rely on ChromeAppHostDistribution being around (always added when Chrome is > installed), which is used to specify shortcut location, icon exe, icon index. > This allows App Launcher shortcut to be created by the installer. To discern > Chromium / Chrome / SxS I used localized ugliness, thus reducing # file changes. > For app shortcuts, I envision this to be added in shell_util.cc, and not > dist-dependent. Also, I use ChromeAppHostDistribution's GetAppShortcutName() > instead of giving GetAppListShortcutName() to everyone. Wait... isn't ChromeAppHostDistribution going away -- doesn't make much sense to keep that distribution around imo since it goes hand-in-hand with Chrome now..? It makes sense to me to rely on different distributions for the AppLauncher shortcut name for GoogleChrome, Chromium, and Canary as is currently done in this CL. I'll look at the CL in detail tomorrow, I forget what the original comments on https://codereview.chromium.org/13940006/ were that drove this, but maybe making a smaller CL first to address the nits and then make this one to address the leftover? Does that make sense, maybe this is already all there is left to address? Going to bed... I'll take a look tomorrow, but small CLs are good :)! Cheers! Gab
https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... chrome/browser/shell_integration.h:115: // Start Menu/Chrome Apps respectively. Put quotes around these for readability like "Start Menu/Google Chrome". https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... File chrome/browser/ui/views/create_application_shortcut_view.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/create_application_shortcut_view.cc:379: if (menu_check_box_ != NULL && menu_check_box_->checked()) { You will need to make this change in chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.(h|cc). This is only compiled on Linux (hence you didn't notice it on Windows). https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/create_application_shortcut_view.cc:439: create_in_chrome_apps_subdir_ = false; I'm glad this comment came in handy :) https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/create_application_shortcut_view.h:81: bool create_in_chrome_apps_subdir_; Comment here: // If false, the shortcut will be created in the root level of the Start Menu. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... File chrome/browser/web_applications/web_app_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... chrome/browser/web_applications/web_app_win.cc:305: string16 start_menu_subdir = GetAppShortcutsSubdirName(); Move this line below to line 325 (start_menu_subdir is no longer needed up here). and .... (see below comment) https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... chrome/browser/web_applications/web_app_win.cc:328: chrome_apps_dir = chrome_apps_dir.Append(start_menu_subdir); .... why are you still using GetAppShortcutsSubdirName? It seems like you've abstracted it away to ShellUtil in all other uses. I think this line should look more like the new code in GetShortcutPaths -- can you use ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, ....) to get this path? I realise it's much longer than GetAppShortcutsSubdirName but it would be more consistent. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1403: if (location == SHORTCUT_LOCATION_START_MENU_CHROME_DIR) { Can you do this in the switch instead of having this separate if statement down here?
Comments below. Cheers! Gab https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:104: int GetAppListIconIndex() { You can remove this method (see comment on browser_distribution). https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:117: string16 GetAppListShortcutName() { Remove this method and change users to use the distribution. https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:238: base::FilePath shortcut_file = nit: I prefered old name "shortcut_to_pin" https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:240: .AddExtension(installer::kLnkExt); nit: Keep '.' on previous line https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:241: if (base::win::CreateOrUpdateShortcutLink(shortcut_file, nit: indent all parameters at paran or 4 spaces in. https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:375: string16 GetAppListIconPath(); You can remove this method (see suggestion on browser_distribution). https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... chrome/browser/web_applications/web_app.cc:232: string16 GetAppShortcutsSubdirName() { You shouldn't need this method, just use ShellUtil::GetShortcutPath(). https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_win.cc:305: string16 start_menu_subdir = GetAppShortcutsSubdirName(); On 2013/05/15 05:19:45, Matt Giuca wrote: > Move this line below to line 325 (start_menu_subdir is no longer needed up > here). > > and .... (see below comment) In fact delete this line (see comment below). https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_win.cc:328: chrome_apps_dir = chrome_apps_dir.Append(start_menu_subdir); On 2013/05/15 05:19:45, Matt Giuca wrote: > .... why are you still using GetAppShortcutsSubdirName? It seems like you've > abstracted it away to ShellUtil in all other uses. I think this line should look > more like the new code in GetShortcutPaths -- can you use > ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, > ....) to get this path? > > I realise it's much longer than GetAppShortcutsSubdirName but it would be more > consistent. +1 https://codereview.chromium.org/13864015/diff/44001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/installer/setup/in... chrome/installer/setup/install.cc:80: message.append("Start menu "); s/"Start menu "/"Start menu/Google Chrome " https://codereview.chromium.org/13864015/diff/44001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:356: if (!ShellUtil::RemoveShortcuts( On 2013/05/15 02:33:46, huangs1 wrote: > How about SHORTCUT_LOCATION_START_MENU_CHROME_DIR and > SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR? Yes, please add deletion of the SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR as well (or, even better, make a small CL after this one to do this and fix http://crbug.com/238895; instead of making this CL even bigger...). SHORTCUT_LOCATION_START_MENU_ROOT also needs to be purged of v1 apps shortcuts (it never was...). We are getting to a point where we have a lot of calls here and with huangs@'s recent addition of BatchShortcutAction in ShellUtil, most the logic shifted away from here. I think this code can now simply iterate over all the ShortcutLocations and call RemoveShortcuts on them (RemoveShortcuts knows which set of shortcut actions to apply :), thanks Sam!). But again, I'd keep that for another CL. https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/bro... File chrome/installer/util/browser_distribution.h (right): https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/bro... chrome/installer/util/browser_distribution.h:66: virtual string16 GetAppShortcutsSubdirName(); I talked about this with huangs@ and we came up with the following facts and conclusion: 1) You will also need the icon paths + icon indexes in the distribution. 2) This would add a lot of calls :(, but here is how we mitigate this: In BrowserDistribution: A) Make a ShortcutInfo GetShortcutInfo(ShortcutEnum shortcut_enum); method; where a) ShortcutInfo is a struct containing (shortcut_name, icon_path, icon_index) -- effectively merging the current (GetAppShortcutName, GetIconIndex, and GetIconFilename) into a single call. b) ShortcutEnum is an enum (e.g., SHORTCUT_CHROME, SHORTCUT_ALTERNATE_CHROME, and now SHORTCUT_APP_LAUNCHER). B) Add a GetStartMenuShortcutSubfolder(SubfolderEnum subfolder_enum) method to return the appropriate folder (effectively a duplicate of the current GetAppShortcutName() used for both purposes); where a) SubfolderEnum is one of SUBFOLDER_CHROME, SUBFOLDER_APPS This will avoid creating 3 more calls just for App launcher while keeping the desired behavior. 3) You should do this in a precursor CL (mentioning this one in the description for reference if anyone wonders why this is done); this will make it easier to review both that CL and this one after since the refactoring will likely touch files unrelated to this CL. https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (left): https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1393: nit: I'd leave this empty line to separate from the switch block. https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1403: if (location == SHORTCUT_LOCATION_START_MENU_CHROME_DIR) { On 2013/05/15 05:19:45, Matt Giuca wrote: > Can you do this in the switch instead of having this separate if statement down > here? Should be easier with single GetStartMenuSubfolder() call suggested for BrowserDistribution. https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/she... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/44001/chrome/installer/util/she... chrome/installer/util/shell_util.h:327: // SHORTCUT_LOCATION_START_MENU_CHROME_DIR or On 2013/05/15 02:33:46, huangs1 wrote: > Add "," back; I think Oxford comma improves readability for code. +1
https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/browser/ui/views/a... chrome/browser/ui/views/app_list/app_list_controller_win.cc:240: .AddExtension(installer::kLnkExt); On 2013/05/15 21:26:38, gab wrote: > nit: Keep '.' on previous line And indent 4.
Ping, what's up with this CL?
Linux and mac changes will need to be done. I'll do them after the Windows changes are finalized. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... File chrome/browser/shell_integration.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... chrome/browser/shell_integration.cc:42: in_applications_menu(false), On 2013/05/15 02:33:46, huangs1 wrote: > in_application_menu_root Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... File chrome/browser/shell_integration.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... chrome/browser/shell_integration.h:115: // Start Menu/Chrome Apps respectively. On 2013/05/15 05:19:45, Matt Giuca wrote: > Put quotes around these for readability like "Start Menu/Google Chrome". Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/she... chrome/browser/shell_integration.h:116: bool in_applications_menu; On 2013/05/15 02:33:46, huangs1 wrote: > in_application_menu_root Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/app_list/app_list_controller_win.cc:104: int GetAppListIconIndex() { On 2013/05/15 21:26:38, gab wrote: > You can remove this method (see comment on browser_distribution). Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/app_list/app_list_controller_win.cc:117: string16 GetAppListShortcutName() { On 2013/05/15 21:26:38, gab wrote: > Remove this method and change users to use the distribution. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/app_list/app_list_controller_win.cc:238: base::FilePath shortcut_file = On 2013/05/15 21:26:38, gab wrote: > nit: I prefered old name "shortcut_to_pin" Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/app_list/app_list_controller_win.cc:240: .AddExtension(installer::kLnkExt); On 2013/05/15 21:26:38, gab wrote: > nit: Keep '.' on previous line Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/app_list/app_list_controller_win.cc:240: .AddExtension(installer::kLnkExt); On 2013/05/16 00:31:05, Matt Giuca wrote: > On 2013/05/15 21:26:38, gab wrote: > > nit: Keep '.' on previous line > > And indent 4. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/app_list/app_list_controller_win.cc:241: if (base::win::CreateOrUpdateShortcutLink(shortcut_file, On 2013/05/15 21:26:38, gab wrote: > nit: indent all parameters at paran or 4 spaces in. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/app_list/app_list_controller_win.cc:375: string16 GetAppListIconPath(); On 2013/05/15 21:26:38, gab wrote: > You can remove this method (see suggestion on browser_distribution). This still needs to exist I think. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... File chrome/browser/ui/views/create_application_shortcut_view.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/ui/... chrome/browser/ui/views/create_application_shortcut_view.h:81: bool create_in_chrome_apps_subdir_; On 2013/05/15 05:19:45, Matt Giuca wrote: > Comment here: // If false, the shortcut will be created in the root level of the > Start Menu. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... File chrome/browser/web_applications/web_app.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... chrome/browser/web_applications/web_app.cc:232: string16 GetAppShortcutsSubdirName() { On 2013/05/15 21:26:38, gab wrote: > You shouldn't need this method, just use ShellUtil::GetShortcutPath(). As far as I'm aware, I can't use BrowserDistribution here? This code is used on mac and linux. I guess I should put an #if(!OS_WIN) around this function so that the windows implementation doesn't use it? https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... File chrome/browser/web_applications/web_app_win.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... chrome/browser/web_applications/web_app_win.cc:305: string16 start_menu_subdir = GetAppShortcutsSubdirName(); On 2013/05/15 05:19:45, Matt Giuca wrote: > Move this line below to line 325 (start_menu_subdir is no longer needed up > here). > > and .... (see below comment) Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/browser/web... chrome/browser/web_applications/web_app_win.cc:328: chrome_apps_dir = chrome_apps_dir.Append(start_menu_subdir); On 2013/05/15 05:19:45, Matt Giuca wrote: > .... why are you still using GetAppShortcutsSubdirName? It seems like you've > abstracted it away to ShellUtil in all other uses. I think this line should look > more like the new code in GetShortcutPaths -- can you use > ShellUtil::GetShortcutPath(ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, > ....) to get this path? > > I realise it's much longer than GetAppShortcutsSubdirName but it would be more > consistent. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/s... chrome/installer/setup/install.cc:80: message.append("Start menu "); On 2013/05/15 21:26:38, gab wrote: > s/"Start menu "/"Start menu/Google Chrome " Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/s... chrome/installer/setup/uninstall.cc:356: if (!ShellUtil::RemoveShortcuts( On 2013/05/15 21:26:38, gab wrote: > On 2013/05/15 02:33:46, huangs1 wrote: > > How about SHORTCUT_LOCATION_START_MENU_CHROME_DIR and > > SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR? > > Yes, please add deletion of the SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR as > well (or, even better, make a small CL after this one to do this and fix > http://crbug.com/238895; instead of making this CL even bigger...). > > SHORTCUT_LOCATION_START_MENU_ROOT also needs to be purged of v1 apps shortcuts > (it never was...). > > We are getting to a point where we have a lot of calls here and with huangs@'s > recent addition of BatchShortcutAction in ShellUtil, most the logic shifted away > from here. > > I think this code can now simply iterate over all the ShortcutLocations and call > RemoveShortcuts on them (RemoveShortcuts knows which set of shortcut actions to > apply :), thanks Sam!). > > But again, I'd keep that for another CL. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... File chrome/installer/util/browser_distribution.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/browser_distribution.h:66: virtual string16 GetAppShortcutsSubdirName(); On 2013/05/15 21:26:38, gab wrote: > I talked about this with huangs@ and we came up with the following facts and > conclusion: > > 1) You will also need the icon paths + icon indexes in the distribution. > 2) This would add a lot of calls :(, but here is how we mitigate this: > > In BrowserDistribution: > A) Make a > ShortcutInfo GetShortcutInfo(ShortcutEnum shortcut_enum); > method; where > a) ShortcutInfo is a struct containing (shortcut_name, icon_path, icon_index) -- > effectively merging the current (GetAppShortcutName, GetIconIndex, and > GetIconFilename) into a single call. > b) ShortcutEnum is an enum (e.g., SHORTCUT_CHROME, SHORTCUT_ALTERNATE_CHROME, > and now SHORTCUT_APP_LAUNCHER). > > B) Add a GetStartMenuShortcutSubfolder(SubfolderEnum subfolder_enum) method to > return the appropriate folder (effectively a duplicate of the current > GetAppShortcutName() used for both purposes); where > a) SubfolderEnum is one of SUBFOLDER_CHROME, SUBFOLDER_APPS > > This will avoid creating 3 more calls just for App launcher while keeping the > desired behavior. > > 3) You should do this in a precursor CL (mentioning this one in the description > for reference if anyone wonders why this is done); this will make it easier to > review both that CL and this one after since the refactoring will likely touch > files unrelated to this CL. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.cc (left): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1393: On 2013/05/15 21:26:38, gab wrote: > nit: I'd leave this empty line to separate from the switch block. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.cc:1403: if (location == SHORTCUT_LOCATION_START_MENU_CHROME_DIR) { On 2013/05/15 05:19:45, Matt Giuca wrote: > Can you do this in the switch instead of having this separate if statement down > here? Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... File chrome/installer/util/shell_util.h (right): https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.h:327: // SHORTCUT_LOCATION_START_MENU_CHROME_DIR or On 2013/05/15 02:33:46, huangs1 wrote: > Add "," back; I think Oxford comma improves readability for code. Done. https://chromiumcodereview.appspot.com/13864015/diff/44001/chrome/installer/u... chrome/installer/util/shell_util.h:543: On 2013/05/15 02:33:46, huangs1 wrote: > Delete extra line. Done.
lg though I'm curious about the Mac/Linux case vs BD now. https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... chrome/browser/web_applications/web_app.cc:232: string16 GetAppShortcutsSubdirName() { On 2013/08/27 07:59:35, calamity wrote: > On 2013/05/15 21:26:38, gab wrote: > > You shouldn't need this method, just use ShellUtil::GetShortcutPath(). > > As far as I'm aware, I can't use BrowserDistribution here? This code is used on > mac and linux. I guess I should put an #if(!OS_WIN) around this function so that > the windows implementation doesn't use it? Ah, right, installer_util target is linked on non-windows platform though I believe; I'm not exactly sure, but I think it's okay to use BD on non-Win platforms. https://codereview.chromium.org/13864015/diff/83001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/13864015/diff/83001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_win.cc:461: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, nit: indent 2 more spaces https://codereview.chromium.org/13864015/diff/83001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_win.cc:477: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, nit: indent 2 more spaces https://codereview.chromium.org/13864015/diff/83001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/83001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:362: Also want to add an extra block here to remove Chrome shortcuts in SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR and SHORTCUT_LOCATION_START_MENU_ROOT (old v1 apps). In fact calling RemoveShortcuts in a loop over all ShellUtil::ShortcutLocations probably makes more sense at this point than this series of ifs. https://codereview.chromium.org/13864015/diff/83001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/13864015/diff/83001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1400: return true; Also collapse above two cases into the same case. https://codereview.chromium.org/13864015/diff/83001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1401: case SHORTCUT_LOCATION_START_MENU_ROOT: Add explicit comment after falling through case statement, e.g.: case SHORTCUT_LOCATION_START_MENU_ROOT: // Falls through. for all cases falling through.
Hey Chris, any update on this? This CL was just brought up @ https://codereview.chromium.org/23581012/diff/1/chrome/installer/setup/uninst... ; Sam wants to see this land before doing further changes... I very much appreciate you doing this work, but it's difficult to work with 4 months long refactorings staying up in the air... since we both: 1) don't want to redo your work 2) but also don't want to introduce a workaround since we know this is coming... Thanks for providing an update! Cheers, Gab
Hey, this fell off my radar a little and I've been unable to work on linux/win over the past 2 weeks due to offsites. I'm currently working to get something in for branch point but I'll revisit this as soon as that's done. Sorry for the inconvenience.
https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... chrome/browser/web_applications/web_app.cc:232: string16 GetAppShortcutsSubdirName() { On 2013/08/28 19:52:27, gab wrote: > On 2013/08/27 07:59:35, calamity wrote: > > On 2013/05/15 21:26:38, gab wrote: > > > You shouldn't need this method, just use ShellUtil::GetShortcutPath(). > > > > As far as I'm aware, I can't use BrowserDistribution here? This code is used > on > > mac and linux. I guess I should put an #if(!OS_WIN) around this function so > that > > the windows implementation doesn't use it? > > Ah, right, installer_util target is linked on non-windows platform though I > believe; I'm not exactly sure, but I think it's okay to use BD on non-Win > platforms. It didn't work on linux. The installer_util target exists but doesn't include browser_distribution as a source. I tried changing the gyp files but there's a lot that includes of windows.h, etc. Is there anyone working on making BrowserDistribution work across different platforms? shell_integration_linux needs to get the information that GetAppShortcutsSubdirName() provides. It was getting it through the applications_menu_subdir string but that's getting removed here. I guess we could copy the contents of GetAppShortcutsSubdirName() into web_app_mac.mm and shell_integration_linux.mm so they both know how to get their "Chrome Apps" directory name. WDYT?
On 2013/09/20 05:54:52, calamity wrote: > https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... > File chrome/browser/web_applications/web_app.cc (right): > > https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applic... > chrome/browser/web_applications/web_app.cc:232: string16 > GetAppShortcutsSubdirName() { > On 2013/08/28 19:52:27, gab wrote: > > On 2013/08/27 07:59:35, calamity wrote: > > > On 2013/05/15 21:26:38, gab wrote: > > > > You shouldn't need this method, just use ShellUtil::GetShortcutPath(). > > > > > > As far as I'm aware, I can't use BrowserDistribution here? This code is used > > on > > > mac and linux. I guess I should put an #if(!OS_WIN) around this function so > > that > > > the windows implementation doesn't use it? > > > > Ah, right, installer_util target is linked on non-windows platform though I > > believe; I'm not exactly sure, but I think it's okay to use BD on non-Win > > platforms. > > It didn't work on linux. The installer_util target exists but doesn't include > browser_distribution as a source. I tried changing the gyp files but there's a > lot that includes of windows.h, etc. Is there anyone working on making > BrowserDistribution work across different platforms? > > shell_integration_linux needs to get the information that > GetAppShortcutsSubdirName() provides. It was getting it through the > applications_menu_subdir string but that's getting removed here. I guess we > could copy the contents of GetAppShortcutsSubdirName() into web_app_mac.mm and > shell_integration_linux.mm so they both know how to get their "Chrome Apps" > directory name. > > WDYT? Arg, that really sucks (BD not being cross-platform; it should really be a bunch of platform independent constants...)... anyways, that's way beyond the point of this CL. Can we maybe have a common method used by BD with a TODO to move it into BD? Otherwise I guess I'm ok with duplicating logic in shell_integration_linux.mm.... meh :(. Gab
I'm still planning to refactor BD, just punting on it for the time being. =) -- Samuel Huang On Sep 20, 2013 1:58 AM, <gab@chromium.org> wrote: > On 2013/09/20 05:54:52, calamity wrote: > > https://codereview.chromium.**org/13864015/diff/44001/** > chrome/browser/web_**applications/web_app.cc<https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applications/web_app.cc> > >> File chrome/browser/web_**applications/web_app.cc (right): >> > > > https://codereview.chromium.**org/13864015/diff/44001/** > chrome/browser/web_**applications/web_app.cc#**newcode232<https://codereview.chromium.org/13864015/diff/44001/chrome/browser/web_applications/web_app.cc#newcode232> > >> chrome/browser/web_**applications/web_app.cc:232: string16 >> GetAppShortcutsSubdirName() { >> On 2013/08/28 19:52:27, gab wrote: >> > On 2013/08/27 07:59:35, calamity wrote: >> > > On 2013/05/15 21:26:38, gab wrote: >> > > > You shouldn't need this method, just use >> ShellUtil::GetShortcutPath(). >> > > >> > > As far as I'm aware, I can't use BrowserDistribution here? This code >> is >> > used > >> > on >> > > mac and linux. I guess I should put an #if(!OS_WIN) around this >> function >> > so > >> > that >> > > the windows implementation doesn't use it? >> > >> > Ah, right, installer_util target is linked on non-windows platform >> though I >> > believe; I'm not exactly sure, but I think it's okay to use BD on >> non-Win >> > platforms. >> > > It didn't work on linux. The installer_util target exists but doesn't >> include >> browser_distribution as a source. I tried changing the gyp files but >> there's a >> lot that includes of windows.h, etc. Is there anyone working on making >> BrowserDistribution work across different platforms? >> > > shell_integration_linux needs to get the information that >> GetAppShortcutsSubdirName() provides. It was getting it through the >> applications_menu_subdir string but that's getting removed here. I guess >> we >> could copy the contents of GetAppShortcutsSubdirName() into >> web_app_mac.mm and >> shell_integration_linux.mm so they both know how to get their "Chrome >> Apps" >> directory name. >> > > WDYT? >> > > Arg, that really sucks (BD not being cross-platform; it should really be a > bunch > of platform independent constants...)... anyways, that's way beyond the > point of > this CL. > > Can we maybe have a common method used by BD with a TODO to move it into > BD? > Otherwise I guess I'm ok with duplicating logic in > shell_integration_linux.mm.... meh :(. > > Gab > > https://codereview.chromium.**org/13864015/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Made this work on linux and mac. https://codereview.chromium.org/13864015/diff/83001/chrome/browser/web_applic... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/13864015/diff/83001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_win.cc:461: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, On 2013/08/28 19:52:27, gab wrote: > nit: indent 2 more spaces Done. https://codereview.chromium.org/13864015/diff/83001/chrome/browser/web_applic... chrome/browser/web_applications/web_app_win.cc:477: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, On 2013/08/28 19:52:27, gab wrote: > nit: indent 2 more spaces Done. https://codereview.chromium.org/13864015/diff/83001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/83001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:362: On 2013/08/28 19:52:27, gab wrote: > Also want to add an extra block here to remove Chrome shortcuts in > SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR and > SHORTCUT_LOCATION_START_MENU_ROOT (old v1 apps). > > In fact calling RemoveShortcuts in a loop over all ShellUtil::ShortcutLocations > probably makes more sense at this point than this series of ifs. Done. https://codereview.chromium.org/13864015/diff/83001/chrome/installer/util/she... File chrome/installer/util/shell_util.cc (right): https://codereview.chromium.org/13864015/diff/83001/chrome/installer/util/she... chrome/installer/util/shell_util.cc:1401: case SHORTCUT_LOCATION_START_MENU_ROOT: On 2013/08/28 19:52:27, gab wrote: > Add explicit comment after falling through case statement, e.g.: > > case SHORTCUT_LOCATION_START_MENU_ROOT: // Falls through. > > for all cases falling through. Done.
Looking good, comments below. Cheers! Gab https://codereview.chromium.org/13864015/diff/131001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/131001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:401: location != SHORTCUT_LOCATION_END; ++location) { nit: indent +1 space. https://codereview.chromium.org/13864015/diff/131001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:410: if (!ShellUtil::RemoveShortcuts(ShellUtil::SHORTCUT_LOCATION_TASKBAR_PINS, Gah; sucks we have to leave this one out.. this whole logic about user-level vs system-level shortcut should be solved (i.e., chrome_browser_main_win.cc::DoUninstallTasks() shouldn't have to worry about user-level shortcuts and this method should then do the right thing)... Anyways this is beyond the scope of this CL, can you add here: // TODO(gab): Fix user-level shortcut deletion on uninstall. https://codereview.chromium.org/13864015/diff/131001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/131001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:53: SHORTCUT_LOCATION_END, nit: it was suggested to me in a previous code review to not add the trailing comma for enum items which must always remain the last item (making it even more explicit beyond it's name that new items shouldn't go after)... Can you remove the trailing comma here too for that reason? https://codereview.chromium.org/13864015/diff/139001/chrome/browser/apps/shor... File chrome/browser/apps/shortcut_manager.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/apps/shor... chrome/browser/apps/shortcut_manager.cc:44: creation_locations.in_applications_menu_root = true; We don't want it in the root, right? https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... chrome/browser/shell_integration.h:124: bool in_applications_menu_chrome_apps_subdir; I don't think we ever need more than 1 of those at once; how about we make this a single variable with enum? e.g., ApplicationsMenuLocation { APP_LOCATION_NONE, APP_LOCATION_ROOT, APP_LOCATION_SUBDIR_CHROME, APP_LOCATION_SUBDIR_CHROMEAPPS, } https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:286: string16 GetAppShortcutsSubdirName() { I would prefer having only a single copy of this method; how about putting it in shell_integration.cc (maybe in a #if !defined(OS_WIN) block that would have this TODO explaining why this needs to be there for non-Win?). https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux_unittest.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... chrome/browser/shell_integration_linux_unittest.cc:174: string16 GetAppListShortcutName() { Why define this inline here and below?? Does this even compile?! https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... chrome/browser/web_applications/web_app.cc:19: #include "chrome/installer/util/browser_distribution.h" Remove unused include. https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_linux.cc (left): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_linux.cc:58: creation_locations.applications_menu_subdir = GetAppShortcutsSubdirName(); Shouldn't this be replaced by the boolean indicating it should be created? https://codereview.chromium.org/13864015/diff/139001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:350: if (!ShellUtil::UpdateShortcutsWithArgs( Use the same looping logic here to capture all apps shortcuts.
https://codereview.chromium.org/13864015/diff/139001/chrome/browser/apps/shor... File chrome/browser/apps/shortcut_manager.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/apps/shor... chrome/browser/apps/shortcut_manager.cc:44: creation_locations.in_applications_menu_root = true; On 2013/10/03 17:22:49, gab wrote: > We don't want it in the root, right? Done. https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... chrome/browser/shell_integration.h:124: bool in_applications_menu_chrome_apps_subdir; On 2013/10/03 17:22:49, gab wrote: > I don't think we ever need more than 1 of those at once; how about we make this > a single variable with enum? > > e.g., > > ApplicationsMenuLocation { > APP_LOCATION_NONE, > APP_LOCATION_ROOT, > APP_LOCATION_SUBDIR_CHROME, > APP_LOCATION_SUBDIR_CHROMEAPPS, > } Done. https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:286: string16 GetAppShortcutsSubdirName() { On 2013/10/03 17:22:49, gab wrote: > I would prefer having only a single copy of this method; how about putting it in > shell_integration.cc (maybe in a #if !defined(OS_WIN) block that would have > this TODO explaining why this needs to be there for non-Win?). Done. https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux_unittest.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/shell_int... chrome/browser/shell_integration_linux_unittest.cc:174: string16 GetAppListShortcutName() { On 2013/10/03 17:22:49, gab wrote: > Why define this inline here and below?? Does this even compile?! Whoa, how did that happen. removed. https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... chrome/browser/web_applications/web_app.cc:19: #include "chrome/installer/util/browser_distribution.h" On 2013/10/03 17:22:49, gab wrote: > Remove unused include. Done. https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_linux.cc (left): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_linux.cc:58: creation_locations.applications_menu_subdir = GetAppShortcutsSubdirName(); On 2013/10/03 17:22:49, gab wrote: > Shouldn't this be replaced by the boolean indicating it should be created? I don't think so. This just specifies the directory. The boolean value would be populated by GetExistingShortcutLocations(). https://codereview.chromium.org/13864015/diff/139001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/139001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:350: if (!ShellUtil::UpdateShortcutsWithArgs( On 2013/10/03 17:22:49, gab wrote: > Use the same looping logic here to capture all apps shortcuts. Done.
Just went over the entire CL again, looks great, comments below. @calamity: Please run setup_unittests.exe locally (they're not on the waterfall for now...). huangs/grt, PTAL, this is pretty big and I want to make sure we're not forgetting anything..! Cheers! Gab https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_linux.cc (left): https://codereview.chromium.org/13864015/diff/139001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_linux.cc:58: creation_locations.applications_menu_subdir = GetAppShortcutsSubdirName(); On 2013/10/14 02:24:25, calamity wrote: > On 2013/10/03 17:22:49, gab wrote: > > Shouldn't this be replaced by the boolean indicating it should be created? > > I don't think so. This just specifies the directory. The boolean value would be > populated by GetExistingShortcutLocations(). Well the previous comment seem to say that we want to create this even if it no longer exists..? https://codereview.chromium.org/13864015/diff/178001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:130: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_DIR, Also add CHROME_APPS dir here. I think this code is redundant at this point, but would need to investigate more, definitely beyond the scope of this CL. Feel free to add on line 127: // TODO(gab): Look into removing this code which is now redundant with the work done by setup.exe on uninstall. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.cc:120: chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); I think this can be inlined below and still fit on a single line in the if. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.h:110: // and "Start Menu/Chrome Apps" respectively. What about for other platforms? This is used on Linux too, no? If not maybe we should #ifdef this member to Windows only. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.h:130: bool in_quick_launch_bar; #ifdef this to Windows only? https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.h:136: bool hidden; Should we #ifdef this to Linux only then? https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:44: #include "ui/base/l10n/l10n_util.h" Includes no longer required. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:854: ShellIntegration::APP_MENU_LOCATION_NONE || nit: indent this line 4 spaces as it's a continuation of the previous statement https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:891: ShellIntegration::APP_MENU_LOCATION_NONE || nit: indent 4 spaces in here too https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:896: ShellIntegration::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS) { nit: indent 4 spaces https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/browse... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/browse... chrome/browser/ui/browser_browsertest.cc:340: #if defined(OS_WIN) Wait... isn't this part of the other change you're working on, it shouldn't be in this one... right?! https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/gtk/cr... File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/gtk/cr... chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:214: if (create_in_chrome_apps_subdir_) { I think this is simpler as: creation_locations.applications_menu_location = create_in_chrome_apps_subdir_ ? ShellIntegration::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS : ShellIntegration::APP_MENU_LOCATION_ROOT; https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.cc:375: if (create_in_chrome_apps_subdir_) { Use the ternary operator here as well (as suggested in create_application_shortcuts_dialog_gtk.cc). https://codereview.chromium.org/13864015/diff/178001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_win.cc:506: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_DIR, I like the trailing comma in general, but SHORTCUT_LOCATION_START_MENU_ROOT and SHORTCUT_LOCATION_QUICK_LAUNCH entries don't use it... either always or never use trailing comma. Previous code didn't use it, so maybe err on the side of being consistent with that. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:389: if (!ShellUtil::RemoveShortcuts(ShellUtil::SHORTCUT_LOCATION_TASKBAR_PINS, SHORTCUT_LOCATION_TASKBAR_PINS will be processed above as well. GetShortcutPaths always returns CURRENT_USER for SHORTCUT_LOCATION_TASKBAR_PINS: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u... So I think it's fine to remove this block altogether. On user-level installs this won't make a difference and on system-level installs it will do the right thing (although we should really just do a second pass at CURRENT_USER for system-level installs; instead of relying on chrome_browser_main.cc as I highlighted there). https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:519: // If |location| is SHORTCUT_LOCATION_START_MENU_CHROME_DIR, the shortcut It is no longer true that this is only SHORTCUT_LOCATION_START_MENU_CHROME_DIR and I don't think it makes sense either to list every location that is a folder; perhaps just state that if |location| is a Chrome-specific folder, it will be deleted as well. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... chrome/installer/util/shell_util_unittest.cc:563: ASSERT_FALSE(base::PathExists(shortcut_folder)); Also make sure we have, or add one if not, a test that ensures we don't delete non-Chrome specific folders (e.g., put shortcuts in ShellUtil::SHORTCUT_LOCATION_START_MENU_ROOT and make sure calling RemoveShortcuts(ShellUtil::SHORTCUT_LOCATION_START_MENU_ROOT, ...) deletes the shortcuts but not the folder).
Mostly nits. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:616: // "APP_MENU_LOCATION_SUBDIR_CHROMEAPPS" depends on whether it contains NIT: I think you can unwrap "APP_MENU_LOCATION_SUBDIR_CHROMEAPPS" to get length exactly 80. Alternatively, you can write "APP_MENU_LOCATION_SUBDIR_CHROMEAPPS" first. :) https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/i... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/i... chrome/installer/setup/install.cc:83: default: Also handle SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, perhaps? https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:356: LOG(WARNING) << "Failed to retarget shortcuts with ShortcutLocation:" NIT: Space after ":". https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:383: LOG(WARNING) << "Failed to delete shortcuts with ShortcutLocation:" NIT: Space after ":". https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:45: SHORTCUT_LOCATION_START = 0, Please consider SHORTCUT_LOCATION_BEGIN, to avoid confusion with ..._START_MENU_..., and to resemble STL .begin() / .end() idiom. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:53: SHORTCUT_LOCATION_END, Please consider NUM_SHORTCUT_LOCATION, without trailing ",", and with comment "// This always appears last.".
Please make the CL description more informative. For example, the installer/util changes seem to amount to: """ Adds support for Chrome app Start Menu shortcut folders. BrowserDistribution now provides the name of a distribution-specific Chrome app subfolder, and ShellUtil's utility functions can now create, update, and remove shortcuts therein. """ I haven't reviewed outside of chrome/installer. I'll take another look once you update the description. Thanks. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/i... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/i... chrome/installer/setup/install.cc:81: message.append("Start menu/Google Chrome "); this portion of the message will be incorrect for canary and chromium installs. can you get the string from |dist|? https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:351: for (int location = ShellUtil::SHORTCUT_LOCATION_START; if you use ShellUtil::ShortcutLocation rather than int here, can you not rid the code of the static_cast below? https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:356: LOG(WARNING) << "Failed to retarget shortcuts with ShortcutLocation:" nit: "with" -> "in" https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:378: for (int location = ShellUtil::SHORTCUT_LOCATION_START; int -> ShellUtil::ShortcutLocation https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:383: LOG(WARNING) << "Failed to delete shortcuts with ShortcutLocation:" nit: "with" -> "in"
Thanks Greg for taking a look; quick reply to some of your comments below. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:351: for (int location = ShellUtil::SHORTCUT_LOCATION_START; On 2013/10/22 13:37:45, grt wrote: > if you use ShellUtil::ShortcutLocation rather than int here, can you not rid the > code of the static_cast below? Yes, but then you can't use ++ (enums are not iterable); you'd have to do location = static_cast<ShellUtil::ShortcutLocation>(location + 1) to increment IIRC... I think the current implementation is cleaner. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:378: for (int location = ShellUtil::SHORTCUT_LOCATION_START; On 2013/10/22 13:37:45, grt wrote: > int -> ShellUtil::ShortcutLocation Same here. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:383: LOG(WARNING) << "Failed to delete shortcuts with ShortcutLocation:" On 2013/10/17 19:13:35, huangs1 wrote: > NIT: Space after ":". Given this is the ID of the shortcut location I think no space is slightly better, e.g., ... in ShortcutLocation:5
https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:351: for (int location = ShellUtil::SHORTCUT_LOCATION_START; On 2013/10/22 17:07:29, gab wrote: > On 2013/10/22 13:37:45, grt wrote: > > if you use ShellUtil::ShortcutLocation rather than int here, can you not rid > the > > code of the static_cast below? > > Yes, but then you can't use ++ (enums are not iterable); you'd have to do > location = static_cast<ShellUtil::ShortcutLocation>(location + 1) to increment > IIRC... > > I think the current implementation is cleaner. agreed. i haven't been writing code enough recently. sigh. c++, you say? ;-)
Ping? I think we're almost done with this, the underlying code has already been shipping in an incorrect state for far too long... this was a "follow-up" CL, can we please get it over with? Thanks, Gab
https://codereview.chromium.org/13864015/diff/178001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main_win.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_win.cc:130: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_DIR, On 2013/10/17 15:13:31, gab wrote: > Also add CHROME_APPS dir here. > > I think this code is redundant at this point, but would need to investigate > more, definitely beyond the scope of this CL. > > Feel free to add on line 127: > // TODO(gab): Look into removing this code which is now redundant with the work > done by setup.exe on uninstall. Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.cc:120: chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel(); On 2013/10/17 15:13:31, gab wrote: > I think this can be inlined below and still fit on a single line in the if. Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.h:110: // and "Start Menu/Chrome Apps" respectively. On 2013/10/17 15:13:31, gab wrote: > What about for other platforms? This is used on Linux too, no? If not maybe we > should #ifdef this member to Windows only. This is used by create_application_shortcuts_view.cc which is in linux_aura. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.h:130: bool in_quick_launch_bar; On 2013/10/17 15:13:31, gab wrote: > #ifdef this to Windows only? Same above. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration.h:136: bool hidden; On 2013/10/17 15:13:31, gab wrote: > Should we #ifdef this to Linux only then? Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:44: #include "ui/base/l10n/l10n_util.h" On 2013/10/17 15:13:31, gab wrote: > Includes no longer required. Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:616: // "APP_MENU_LOCATION_SUBDIR_CHROMEAPPS" depends on whether it contains On 2013/10/17 19:13:35, huangs1 wrote: > NIT: I think you can unwrap "APP_MENU_LOCATION_SUBDIR_CHROMEAPPS" to get length > exactly 80. Alternatively, you can write "APP_MENU_LOCATION_SUBDIR_CHROMEAPPS" > first. :) Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:854: ShellIntegration::APP_MENU_LOCATION_NONE || On 2013/10/17 15:13:31, gab wrote: > nit: indent this line 4 spaces as it's a continuation of the previous statement Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:891: ShellIntegration::APP_MENU_LOCATION_NONE || On 2013/10/17 15:13:31, gab wrote: > nit: indent 4 spaces in here too Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:896: ShellIntegration::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS) { On 2013/10/17 15:13:31, gab wrote: > nit: indent 4 spaces Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/browse... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/browse... chrome/browser/ui/browser_browsertest.cc:340: #if defined(OS_WIN) On 2013/10/17 15:13:31, gab wrote: > Wait... isn't this part of the other change you're working on, it shouldn't be > in this one... right?! Removed. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/gtk/cr... File chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/gtk/cr... chrome/browser/ui/gtk/create_application_shortcuts_dialog_gtk.cc:214: if (create_in_chrome_apps_subdir_) { On 2013/10/17 15:13:31, gab wrote: > I think this is simpler as: > > creation_locations.applications_menu_location = create_in_chrome_apps_subdir_ ? > ShellIntegration::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS : > ShellIntegration::APP_MENU_LOCATION_ROOT; Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/views/... File chrome/browser/ui/views/create_application_shortcut_view.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/ui/views/... chrome/browser/ui/views/create_application_shortcut_view.cc:375: if (create_in_chrome_apps_subdir_) { On 2013/10/17 15:13:31, gab wrote: > Use the ternary operator here as well (as suggested in > create_application_shortcuts_dialog_gtk.cc). Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_win.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_win.cc:506: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_DIR, On 2013/10/17 15:13:31, gab wrote: > I like the trailing comma in general, but SHORTCUT_LOCATION_START_MENU_ROOT and > SHORTCUT_LOCATION_QUICK_LAUNCH entries don't use it... either always or never > use trailing comma. > > Previous code didn't use it, so maybe err on the side of being consistent with > that. Done. https://codereview.chromium.org/13864015/diff/178001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_win.cc:506: ShellUtil::SHORTCUT_LOCATION_START_MENU_CHROME_DIR, On 2013/10/17 15:13:31, gab wrote: > I like the trailing comma in general, but SHORTCUT_LOCATION_START_MENU_ROOT and > SHORTCUT_LOCATION_QUICK_LAUNCH entries don't use it... either always or never > use trailing comma. > > Previous code didn't use it, so maybe err on the side of being consistent with > that. Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/i... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/i... chrome/installer/setup/install.cc:81: message.append("Start menu/Google Chrome "); On 2013/10/22 13:37:45, grt wrote: > this portion of the message will be incorrect for canary and chromium installs. > can you get the string from |dist|? Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/i... chrome/installer/setup/install.cc:83: default: On 2013/10/17 19:13:35, huangs1 wrote: > Also handle SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR, perhaps? Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:356: LOG(WARNING) << "Failed to retarget shortcuts with ShortcutLocation:" On 2013/10/22 13:37:45, grt wrote: > nit: "with" -> "in" Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:356: LOG(WARNING) << "Failed to retarget shortcuts with ShortcutLocation:" On 2013/10/17 19:13:35, huangs1 wrote: > NIT: Space after ":". Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:378: for (int location = ShellUtil::SHORTCUT_LOCATION_START; On 2013/10/22 13:37:45, grt wrote: > int -> ShellUtil::ShortcutLocation Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:378: for (int location = ShellUtil::SHORTCUT_LOCATION_START; On 2013/10/22 17:07:29, gab wrote: > On 2013/10/22 13:37:45, grt wrote: > > int -> ShellUtil::ShortcutLocation > > Same here. Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:383: LOG(WARNING) << "Failed to delete shortcuts with ShortcutLocation:" On 2013/10/22 17:07:29, gab wrote: > On 2013/10/17 19:13:35, huangs1 wrote: > > NIT: Space after ":". > > Given this is the ID of the shortcut location I think no space is slightly > better, e.g., > > ... in ShortcutLocation:5 Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:389: if (!ShellUtil::RemoveShortcuts(ShellUtil::SHORTCUT_LOCATION_TASKBAR_PINS, On 2013/10/17 15:13:31, gab wrote: > SHORTCUT_LOCATION_TASKBAR_PINS will be processed above as well. > > GetShortcutPaths always returns CURRENT_USER for SHORTCUT_LOCATION_TASKBAR_PINS: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/u... > > So I think it's fine to remove this block altogether. > > On user-level installs this won't make a difference and on system-level installs > it will do the right thing (although we should really just do a second pass at > CURRENT_USER for system-level installs; instead of relying on > chrome_browser_main.cc as I highlighted there). > Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:45: SHORTCUT_LOCATION_START = 0, On 2013/10/17 19:13:35, huangs1 wrote: > Please consider SHORTCUT_LOCATION_BEGIN, to avoid confusion with > ..._START_MENU_..., and to resemble STL .begin() / .end() idiom. Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:53: SHORTCUT_LOCATION_END, On 2013/10/17 19:13:35, huangs1 wrote: > Please consider NUM_SHORTCUT_LOCATION, without trailing ",", and with comment > "// This always appears last.". Done. https://codereview.chromium.org/13864015/diff/178001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:519: // If |location| is SHORTCUT_LOCATION_START_MENU_CHROME_DIR, the shortcut On 2013/10/17 15:13:31, gab wrote: > It is no longer true that this is only SHORTCUT_LOCATION_START_MENU_CHROME_DIR > and I don't think it makes sense either to list every location that is a folder; > perhaps just state that if |location| is a Chrome-specific folder, it will be > deleted as well. Done.
On 2013/10/22 13:37:44, grt wrote: > Please make the CL description more informative. For example, the installer/util > changes seem to amount to: > > """ > Adds support for Chrome app Start Menu shortcut folders. > > BrowserDistribution now provides the name of a distribution-specific > Chrome app subfolder, and ShellUtil's utility functions can now create, > update, and remove shortcuts therein. > """ > > I haven't reviewed outside of chrome/installer. I'll take another look once you > update the description. Thanks. Ping.
On 2013/11/20 14:39:15, grt wrote: > On 2013/10/22 13:37:44, grt wrote: > > Please make the CL description more informative. For example, the > installer/util > > changes seem to amount to: > > > > """ > > Adds support for Chrome app Start Menu shortcut folders. > > > > BrowserDistribution now provides the name of a distribution-specific > > Chrome app subfolder, and ShellUtil's utility functions can now create, > > update, and remove shortcuts therein. > > """ > > > > I haven't reviewed outside of chrome/installer. I'll take another look once > you > > update the description. Thanks. > > Ping. Agreed that the CL description needs to be clarified, this has quite diverged from the original simple follow-up cleanup. In fact, is it possible to split a small cleanup into its own CL and do this refactoring to fix app shortcut uninstall in a separate CL (or is the cleanup also using the new BrowserDistribution constructs being introduced here? Sorry, I lost context on this CL by now...). Thanks! Gab
On 2013/11/20 15:35:55, gab wrote: > On 2013/11/20 14:39:15, grt wrote: > > On 2013/10/22 13:37:44, grt wrote: > > > Please make the CL description more informative. For example, the > > installer/util > > > changes seem to amount to: > > > > > > """ > > > Adds support for Chrome app Start Menu shortcut folders. > > > > > > BrowserDistribution now provides the name of a distribution-specific > > > Chrome app subfolder, and ShellUtil's utility functions can now create, > > > update, and remove shortcuts therein. > > > """ > > > > > > I haven't reviewed outside of chrome/installer. I'll take another look once > > you > > > update the description. Thanks. > > > > Ping. > > Agreed that the CL description needs to be clarified, this has quite diverged > from the original simple follow-up cleanup. > > In fact, is it possible to split a small cleanup into its own CL and do this > refactoring to fix app shortcut uninstall in a separate CL (or is the cleanup > also using the new BrowserDistribution constructs being introduced here? Sorry, > I lost context on this CL by now...). > > Thanks! > Gab Updated CL comment. As far as I can see, the original cleanup is subsumed by the structural changes in this patch. The majority of changes are flow-on changes from removing ShellIntegration::ShortcutLocations::in_applications_menu and adding the extra values to ShellUtil::ShortcutLocation. The only auxiliary cleanup stuff is the looping through ShortcutLocation in uninstall.cc. I could also split the BrowserDistribution change into a precursor CL which would add support for SUBFOLDER_APPS. I think splitting the start menu location into 3 folders represented by an enum will have be to be done as a single patch and is what makes up the majority of this patch. WDYT?
Looks great, a few nits below, I will do a final pass over the full diff tomorrow when I'm fully awake (if you had cleaned up the nits by then it would be even better). Also, can you please run setup_unittests.exe and make sure this doesn't introduce new failures (it has existing failures IIRC which need to be fixed before it goes on the waterfall, but it'd be great to make sure we're not introducing any). Once that's done please add a TEST=setup_unittests.exe to the CL description to state to any future reader that you ran these tests and that they can be used as supplemental tests to your CL beyond the waterfall. Thanks a lot for picking this CL back up! Gab https://codereview.chromium.org/13864015/diff/521001/chrome/browser/shell_int... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/13864015/diff/521001/chrome/browser/shell_int... chrome/browser/shell_integration.cc:52: #endif Please change this to: : on_desktop(false), applications_menu_location(APP_MENU_LOCATION_NONE), in_quick_launch_bar(false) #if defined(OS_POSIX) , hidden(false) #endif { i.e. as done elsewhere in the codebase: http://goo.gl/spWFVx https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:45: SHORTCUT_LOCATION_BEGIN = 0, I would prefer SHORTCUT_LOCATION_FIRST here. https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:54: // This always appears last. I think this is a fairly common idiom (I don't think a comment is required, the name and no-leading comma are sufficient IMO. https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:55: NUM_SHORTCUT_LOCATION NUM_SHORTCUT_LOCATIONS
On 2013/11/21 01:58:17, calamity wrote: > On 2013/11/20 15:35:55, gab wrote: > > On 2013/11/20 14:39:15, grt wrote: > > > On 2013/10/22 13:37:44, grt wrote: > > > > Please make the CL description more informative. For example, the > > > installer/util > > > > changes seem to amount to: > > > > > > > > """ > > > > Adds support for Chrome app Start Menu shortcut folders. > > > > > > > > BrowserDistribution now provides the name of a distribution-specific > > > > Chrome app subfolder, and ShellUtil's utility functions can now create, > > > > update, and remove shortcuts therein. > > > > """ > > > > > > > > I haven't reviewed outside of chrome/installer. I'll take another look > once > > > you > > > > update the description. Thanks. > > > > > > Ping. > > > > Agreed that the CL description needs to be clarified, this has quite diverged > > from the original simple follow-up cleanup. > > > > In fact, is it possible to split a small cleanup into its own CL and do this > > refactoring to fix app shortcut uninstall in a separate CL (or is the cleanup > > also using the new BrowserDistribution constructs being introduced here? > Sorry, > > I lost context on this CL by now...). > > > > Thanks! > > Gab > > Updated CL comment. > > As far as I can see, the original cleanup is subsumed by the structural changes > in this patch. The majority of changes are flow-on changes from removing > ShellIntegration::ShortcutLocations::in_applications_menu and adding the extra > values to ShellUtil::ShortcutLocation. The only auxiliary cleanup stuff is the > looping through ShortcutLocation in uninstall.cc. > > I could also split the BrowserDistribution change into a precursor CL which > would add support for SUBFOLDER_APPS. I think splitting the start menu location > into 3 folders represented by an enum will have be to be done as a single patch > and is what makes up the majority of this patch. The major BrowserDistribution was already done in a precursor CL, in this CL you're only adding an extra field to the previously introduced enum, right? I'm all for splitting this up into smaller CLs, but don't just split for the heck of splitting if it doesn't really reduce the complexity/size of this CL overall IMO. Up to you! Thanks! Gab > > WDYT?
Ran setup_unittests and no extra tests fail. Updated CL description. https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:45: SHORTCUT_LOCATION_BEGIN = 0, On 2013/11/21 05:50:19, gab wrote: > I would prefer SHORTCUT_LOCATION_FIRST here. Done. https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:54: // This always appears last. On 2013/11/21 05:50:19, gab wrote: > I think this is a fairly common idiom (I don't think a comment is required, the > name and no-leading comma are sufficient IMO. Removed comment. I don't understand what you mean by 'no-leading comma'. Did this mean leave the blank line? https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:55: NUM_SHORTCUT_LOCATION On 2013/11/21 05:50:19, gab wrote: > NUM_SHORTCUT_LOCATIONS Done.
Awesome, this is so much better and cleaner than the previous code IMO, lgtm w/ comments below. Thanks! Gab https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:54: // This always appears last. On 2013/11/21 06:52:46, calamity wrote: > On 2013/11/21 05:50:19, gab wrote: > > I think this is a fairly common idiom (I don't think a comment is required, > the > > name and no-leading comma are sufficient IMO. > > Removed comment. I don't understand what you mean by 'no-leading comma'. Did > this mean leave the blank line? Nah, remove the blank line; my bad, I meant "no-trailing comma", i.e., usually we ask that the last enum entry have a trailing comma, except when that entry should always remain last. So my point was that given its name and the fact it doesn't have a trailing comma, it's obvious that this should remain last. https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... chrome/browser/shell_integration.cc:51: , hidden = false; nit: Remove ; at the end of this line (I don't think this even compiles) https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:851: // same place ('applications'). This makes me think that "hidden" could also be a location, simplifying the state of ShortcutLocations even more. I'm not requesting that you do this in this CL or at all, but I'm happy to review it if you care to clean it in a follow-up CL. https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux_unittest.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... chrome/browser/shell_integration_linux_unittest.cc:174: ShellIntegration::APP_MENU_LOCATION_NONE); nit: I know this feels backwards, but the EXPECT_EQ macro is EXPECT_EQ(expected, actual); So please flip them to have the constant as the first argument here and below. https://codereview.chromium.org/13864015/diff/721001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_linux.cc (left): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_linux.cc:58: creation_locations.applications_menu_subdir = GetAppShortcutsSubdirName(); I think I've asked this before, but why is this being removed again? Doesn't that alter the logic? https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/i... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/i... chrome/installer/setup/install.cc:82: UTF16ToUTF8(dist->GetStartMenuShortcutSubfolder( nit: Indent this and the lines below to the '(' above if possible. https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:352: location != ShellUtil::NUM_SHORTCUT_LOCATIONS; ++location) { nit: I think the paradigm "location < ShellUtil::NUM_SHORTCUT_LOCATIONS" is more common than "!=" in for loop conditions. https://codereview.chromium.org/13864015/diff/721001/chrome/installer/util/sh... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/installer/util/sh... chrome/installer/util/shell_util_unittest.cc:537: TEST_F(ShellUtilShortcutTest, CreateMultipleStartMenuShortcutsAndRemoveFolder) { Please also create SHORTCUT_LOCATION_START_MENU_CHROME_APPS shorcuts and call RemoveShortcuts for them in this test to have coverage for this new location as well.
+sky for c/b/chrome_browser_main_win.cc and c/b/ui/views OWNERS +benwells for c/b/apps, app_list_service_win.cc and c/b/web_applications OWNERS +erg for c/b/shell_integration_linux* and c/b/ui/gtk OWNERS https://codereview.chromium.org/13864015/diff/521001/chrome/browser/shell_int... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/13864015/diff/521001/chrome/browser/shell_int... chrome/browser/shell_integration.cc:52: #endif On 2013/11/21 05:50:19, gab wrote: > Please change this to: > > : on_desktop(false), > applications_menu_location(APP_MENU_LOCATION_NONE), > in_quick_launch_bar(false) > #if defined(OS_POSIX) > , hidden(false) > #endif > { > > i.e. as done elsewhere in the codebase: http://goo.gl/spWFVx Done. https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... File chrome/installer/util/shell_util.h (right): https://codereview.chromium.org/13864015/diff/521001/chrome/installer/util/sh... chrome/installer/util/shell_util.h:54: // This always appears last. On 2013/11/21 19:52:24, gab wrote: > On 2013/11/21 06:52:46, calamity wrote: > > On 2013/11/21 05:50:19, gab wrote: > > > I think this is a fairly common idiom (I don't think a comment is required, > > the > > > name and no-leading comma are sufficient IMO. > > > > Removed comment. I don't understand what you mean by 'no-leading comma'. Did > > this mean leave the blank line? > > Nah, remove the blank line; my bad, I meant "no-trailing comma", i.e., usually > we ask that the last enum entry have a trailing comma, except when that entry > should always remain last. > > So my point was that given its name and the fact it doesn't have a trailing > comma, it's obvious that this should remain last. Done. https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... File chrome/browser/shell_integration.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... chrome/browser/shell_integration.cc:51: , hidden = false; Done https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... chrome/browser/shell_integration_linux.cc:851: // same place ('applications'). On 2013/11/21 19:52:24, gab wrote: > This makes me think that "hidden" could also be a location, simplifying the > state of ShortcutLocations even more. > > I'm not requesting that you do this in this CL or at all, but I'm happy to > review it if you care to clean it in a follow-up CL. Good point. https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... File chrome/browser/shell_integration_linux_unittest.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/shell_int... chrome/browser/shell_integration_linux_unittest.cc:174: ShellIntegration::APP_MENU_LOCATION_NONE); On 2013/11/21 19:52:24, gab wrote: > nit: I know this feels backwards, but the EXPECT_EQ macro is > > EXPECT_EQ(expected, actual); > > So please flip them to have the constant as the first argument here and below. Done. https://codereview.chromium.org/13864015/diff/721001/chrome/browser/web_appli... File chrome/browser/web_applications/web_app_linux.cc (left): https://codereview.chromium.org/13864015/diff/721001/chrome/browser/web_appli... chrome/browser/web_applications/web_app_linux.cc:58: creation_locations.applications_menu_subdir = GetAppShortcutsSubdirName(); On 2013/11/21 19:52:24, gab wrote: > I think I've asked this before, but why is this being removed again? Doesn't > that alter the logic? GetExistingShortcutLocations used populate creation_locations with creation_locations.in_applications_menu = true. Now that it populates with creation_locations.applications_menu_location = APP_MENU_LOCATION_SUBDIR_CHROMEAPPS, there's no need to set the shortcut directory here (also, the member no longer exists). https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/i... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/i... chrome/installer/setup/install.cc:82: UTF16ToUTF8(dist->GetStartMenuShortcutSubfolder( On 2013/11/21 19:52:24, gab wrote: > nit: Indent this and the lines below to the '(' above if possible. Done. https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:352: location != ShellUtil::NUM_SHORTCUT_LOCATIONS; ++location) { On 2013/11/21 19:52:24, gab wrote: > nit: I think the paradigm "location < ShellUtil::NUM_SHORTCUT_LOCATIONS" is more > common than "!=" in for loop conditions. Done. https://codereview.chromium.org/13864015/diff/721001/chrome/installer/util/sh... File chrome/installer/util/shell_util_unittest.cc (right): https://codereview.chromium.org/13864015/diff/721001/chrome/installer/util/sh... chrome/installer/util/shell_util_unittest.cc:537: TEST_F(ShellUtilShortcutTest, CreateMultipleStartMenuShortcutsAndRemoveFolder) { On 2013/11/21 19:52:24, gab wrote: > Please also create SHORTCUT_LOCATION_START_MENU_CHROME_APPS shorcuts and call > RemoveShortcuts for them in this test to have coverage for this new location as > well. Done.
A few small nits left, still lgtm https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/i... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/i... chrome/installer/setup/install.cc:83: BrowserDistribution::SUBFOLDER_CHROME)) + nit: indent 4 more spaces here. https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/i... chrome/installer/setup/install.cc:89: BrowserDistribution::SUBFOLDER_APPS)) + and here. https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:352: location != ShellUtil::NUM_SHORTCUT_LOCATIONS; ++location) { You marked as done, but looks like you only changed the one on line 379, not this one.
https://codereview.chromium.org/13864015/diff/1001001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/13864015/diff/1001001/chrome/browser/shell_in... chrome/browser/shell_integration.h:110: // and "Start Menu/Chrome Apps" respectively. Could you please add more documentation about what these do on Linux? I stared at shell_integration_linux.cc for a while, but I'm not really sure what these do there...
https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/i... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/i... chrome/installer/setup/install.cc:83: BrowserDistribution::SUBFOLDER_CHROME)) + On 2013/11/25 13:05:10, gab wrote: > nit: indent 4 more spaces here. Done. https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/i... chrome/installer/setup/install.cc:89: BrowserDistribution::SUBFOLDER_APPS)) + On 2013/11/25 13:05:10, gab wrote: > and here. Done. https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/u... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/13864015/diff/831001/chrome/installer/setup/u... chrome/installer/setup/uninstall.cc:352: location != ShellUtil::NUM_SHORTCUT_LOCATIONS; ++location) { On 2013/11/25 13:05:10, gab wrote: > You marked as done, but looks like you only changed the one on line 379, not > this one. Done. https://codereview.chromium.org/13864015/diff/1001001/chrome/browser/shell_in... File chrome/browser/shell_integration.h (right): https://codereview.chromium.org/13864015/diff/1001001/chrome/browser/shell_in... chrome/browser/shell_integration.h:110: // and "Start Menu/Chrome Apps" respectively. On 2013/11/25 22:03:03, Elliot Glaysher wrote: > Could you please add more documentation about what these do on Linux? I stared > at shell_integration_linux.cc for a while, but I'm not really sure what these do > there... Done. Also changed shell_integration_linux.cc to whitelist implemented locations.
lgtm LGTM L.G.T.M.
Patch set 26 lgtm++, thanks!
lgtm
ping: sky for chrome/browser/ui and chrome/chrome_browser_main.cc OWNER (and grt if you want to have a final review) This CL is blocking further app_host refactoring which huangs wants to take on and so we'd like to get it landed ASAP. Thanks! Gab
LGTM
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/13864015/1251001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/13864015/1291001
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/13864015/1291001
Message was sent while issue was closed.
Change committed as 238648 |
