Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12316097:
Added master_preferences to control shortcuts on windows. (Closed)
|
Created:
7 years, 10 months ago by Joao da Silva Modified:
7 years, 9 months ago Reviewers:
gab CC:
chromium-reviews, grt+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdded master_preferences to control shortcuts on windows.
do_not_create_taskbar_shortcut prevents pinning the start menu shortcut to the taskbar on windows 7 and later.
do_not_create_any_shortcuts is a catch-all that prevents the creation of all shortcuts, including the start menu shortcut.
BUG=178076, 174465
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186681
Patch Set 1 #
Total comments: 6
Patch Set 2 : split into 2 prefs #
Total comments: 23
Patch Set 3 : addressed comments #
Total comments: 2
Patch Set 4 : fixed nested conditional #
Messages
Total messages: 13 (0 generated)
Hi Gabriel, I think this bug is a valid feature request, but feel free to push back if there's a good reason to NOT have this feature :-) Please review, thanks!
On 2013/02/25 12:42:58, Joao da Silva wrote: > Hi Gabriel, > > I think this bug is a valid feature request, but feel free to push back if > there's a good reason to NOT have this feature :-) > > Please review, thanks! Hey Joao, saw bug before this this morning, already replied on the bug. Cheers! Gab
LGTM w/ changes below. Cheers! Gab https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... File chrome/installer/util/master_preferences.cc (right): https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... chrome/installer/util/master_preferences.cc:270: // do_not_create_(desktop|quick_launch|start_menu)_shortcut to true. (see comment below first) Highlight here that this will not affect do_not_create_start_menu_shortcut since the legacy "create_all_shortcuts:false" preference didn't prevent creation of the Start Menu shortcut. https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... chrome/installer/util/master_preferences.cc:279: distribution_->SetBoolean( Do not add this to this method as it is only here to support the legacy "create_all_shortcuts:false" preference which only had the effect of not creating desktop+QL shortcuts. https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... File chrome/installer/util/master_preferences_unittest.cc (right): https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... chrome/installer/util/master_preferences_unittest.cc:375: EXPECT_TRUE(do_not_create_start_menu_shortcut); Explicitly EXPECT_FALSE here to be explicit that this is intentionally not affected by the legacy support of "create_all_shortcuts:false".
Actually, I just thought, the taskbar shortcut depends on the Start Menu shortcut. Can you name the preference "do_not_create_start_menu_and_taskbar_shortcuts" instead to be explicit? I chimed in on the bug to check whether that's the desired behavior from the requesting user's pov.
In fact I feel like leaving these two entangled (Start Menu and taskbar shortcuts) and having a single pref to trigger them is asking for trouble... Perhaps we could create a separate shortcut (say in the User Data dir) and pin that instead; leaving the Start Menu independent of the taskbar pin?
On 2013/02/26 21:03:55, gab wrote: > In fact I feel like leaving these two entangled (Start Menu and taskbar > shortcuts) and having a single pref to trigger them is asking for trouble... > > Perhaps we could create a separate shortcut (say in the User Data dir) and pin > that instead; leaving the Start Menu independent of the taskbar pin? Actually the User Data dir doesn't exist yet on install so we don't want to create it just for that. I was thinking we could create the shortcut-to-be-pinned in the Installer directory (i.e. if you have an InstallerState you can get this from installer_state.GetInstallerDirectory()), but then realized when discussing with robertshield and grt that this won't work either since the Installer directory is inside the version directory (which changes every auto-update)... There are other ways I can think of, but overall it would be a non-trivial change for such a simple request. The consumer's goal is clearly to prevent creation of all shortcuts however (they probably didn't mention the taskbar pin simply because they're on XP or something); so how about simply adding a "do_not_create_any_shortcuts" preference which is read at the top of installer::CreateOrUpdateShortcuts() and forces an early return if set to true? Other distributions can still use the finer grain shortcut preferences (and we can consider adding a Start Menu or taskbar only one if there is demand for it later; but for now there is no point splitting this dependency to achieve something that no one has requested yet...). I'm not a fan of a preference that overrides other preferences, but I think this one is justified. Cheers! Gab
Thanks for the review and the comments! I've implemented the latest proposal on issue 174465. The issue description has also been updated. PTAL. https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... File chrome/installer/util/master_preferences.cc (right): https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... chrome/installer/util/master_preferences.cc:270: // do_not_create_(desktop|quick_launch|start_menu)_shortcut to true. On 2013/02/26 20:35:46, gab wrote: > (see comment below first) > > Highlight here that this will not affect do_not_create_start_menu_shortcut since > the legacy "create_all_shortcuts:false" preference didn't prevent creation of > the Start Menu shortcut. Done. https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... chrome/installer/util/master_preferences.cc:279: distribution_->SetBoolean( On 2013/02/26 20:35:46, gab wrote: > Do not add this to this method as it is only here to support the legacy > "create_all_shortcuts:false" preference which only had the effect of not > creating desktop+QL shortcuts. Done. https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... File chrome/installer/util/master_preferences_unittest.cc (right): https://codereview.chromium.org/12316097/diff/1/chrome/installer/util/master_... chrome/installer/util/master_preferences_unittest.cc:375: EXPECT_TRUE(do_not_create_start_menu_shortcut); On 2013/02/26 20:35:46, gab wrote: > Explicitly EXPECT_FALSE here to be explicit that this is intentionally not > affected by the legacy support of "create_all_shortcuts:false". Done.
Comments below. Thanks! Gab https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... chrome/installer/setup/install.cc:439: // Currently, |do_not_create_any_shortcuts| only affects the creation of the I was seeing more "do_not_create_any_shortcuts" as a meta pref to rule them all (i.e., if you have only that one it still prevents creation of any shortcuts (i.e., returns early from this method)). The other preferences are useful for people who want to tweak which shortcut to not create instead of using the big "do_not_create_any_shortcuts" hammer. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... chrome/installer/setup/install.cc:451: start_menu_properties.set_pin_to_taskbar(!do_not_create_taskbar_shortcut); Shouldn't set this bit if (!do_not_create_taskbar_shortcut). https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences.cc:283: void MasterPreferences::EnforceShorcutPreferences() { As mentioned in install.cc I would prefer that "do_not_create_any_shortcuts" simply result in an early return from CreateOrUpdateShortcuts() instead of actually enabling the other prefs. The less coupled preferences are the easier to maintain, i.e.: with this design if we want to later add do_not_create_start_menu_shortcut we have to make sure it is enforced here; where as if do_not_create_any_shortcuts only causes an early return it automatically enforces all the do_not_create_*_shortcut without explicitly coupling them. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences_constants.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.cc:33: const char kDoNotCreateAnyShortcuts[] = "do_not_create_any_shortcuts"; Please keep the alphabetical order here (i.e. kDoNotCreateAnyShortcuts before kDoNotCreateDesktopShortcut). https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences_constants.h (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.h:69: // Boolean. Prevents the creation of all shortcuts to chrome, including the s/Prevents/Prevent (for consistency with surrounding comments) s/the creation/creation (although in this case I agree that "the creation" is more correct... feel free to instead correct the surrounding comments) https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.h:70: // desktop, quick launch, taskbar and the start menu. desktop, quick launch, taskbar and the start menu. --> desktop, quick launch, taskbar, and the start menu shortcuts. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.h:71: extern const char kDoNotCreateAnyShortcuts[]; Please keep the alphabetical order here (i.e. kDoNotCreateAnyShortcuts before kDoNotCreateDesktopShortcut). https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences_unittest.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:62: " \"do_not_create_any_shortcuts\": true,\n" (order as in declaration) https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:90: installer::master_preferences::kDoNotCreateAnyShortcuts, (same here) https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:377: EXPECT_FALSE(do_not_create_taskbar_shortcut); Add a comment above the 3 expectations explicitly stating that "create_all_shortcut": false should only enforce do_not_create_desktop_shortcut and do_not_create_quick_launch_shortcut https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:434: TEST_F(MasterPreferencesTest, "create_all_shortcuts" is an old preference that has no other impact than enforcing "do_not_create_quick_launch_shortcut" and "do_not_create_desktop_shortcut" when explicitly set to false. I don't think this test is necessary (especially after addressing the other comments).
Thanks for the quick review! Ready for another round :-) https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... chrome/installer/setup/install.cc:439: // Currently, |do_not_create_any_shortcuts| only affects the creation of the On 2013/03/06 20:20:32, gab wrote: > I was seeing more "do_not_create_any_shortcuts" as a meta pref to rule them all > (i.e., if you have only that one it still prevents creation of any shortcuts > (i.e., returns early from this method)). > Makes sense, done. > The other preferences are useful for people who want to tweak which shortcut to > not create instead of using the big "do_not_create_any_shortcuts" hammer. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... chrome/installer/setup/install.cc:451: start_menu_properties.set_pin_to_taskbar(!do_not_create_taskbar_shortcut); On 2013/03/06 20:20:32, gab wrote: > Shouldn't set this bit if (!do_not_create_taskbar_shortcut). Done (this was just setting it to false in that case, but an explicit if is clearer) https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences.cc:283: void MasterPreferences::EnforceShorcutPreferences() { On 2013/03/06 20:20:32, gab wrote: > As mentioned in install.cc I would prefer that "do_not_create_any_shortcuts" > simply result in an early return from CreateOrUpdateShortcuts() instead of > actually enabling the other prefs. > > The less coupled preferences are the easier to maintain, i.e.: with this design > if we want to later add do_not_create_start_menu_shortcut we have to make sure > it is enforced here; where as if do_not_create_any_shortcuts only causes an > early return it automatically enforces all the do_not_create_*_shortcut without > explicitly coupling them. Agreed, done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences_constants.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.cc:33: const char kDoNotCreateAnyShortcuts[] = "do_not_create_any_shortcuts"; On 2013/03/06 20:20:32, gab wrote: > Please keep the alphabetical order here (i.e. kDoNotCreateAnyShortcuts before > kDoNotCreateDesktopShortcut). Done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences_constants.h (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.h:69: // Boolean. Prevents the creation of all shortcuts to chrome, including the On 2013/03/06 20:20:32, gab wrote: > s/Prevents/Prevent (for consistency with surrounding comments) > s/the creation/creation (although in this case I agree that "the creation" is > more correct... feel free to instead correct the surrounding comments) Done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.h:70: // desktop, quick launch, taskbar and the start menu. On 2013/03/06 20:20:32, gab wrote: > desktop, quick launch, taskbar and the start menu. > --> > desktop, quick launch, taskbar, and the start menu shortcuts. Done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_constants.h:71: extern const char kDoNotCreateAnyShortcuts[]; On 2013/03/06 20:20:32, gab wrote: > Please keep the alphabetical order here (i.e. kDoNotCreateAnyShortcuts before > kDoNotCreateDesktopShortcut). Done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... File chrome/installer/util/master_preferences_unittest.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:62: " \"do_not_create_any_shortcuts\": true,\n" On 2013/03/06 20:20:32, gab wrote: > (order as in declaration) Done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:90: installer::master_preferences::kDoNotCreateAnyShortcuts, On 2013/03/06 20:20:32, gab wrote: > (same here) Done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:377: EXPECT_FALSE(do_not_create_taskbar_shortcut); On 2013/03/06 20:20:32, gab wrote: > Add a comment above the 3 expectations explicitly stating that > "create_all_shortcut": false should only enforce do_not_create_desktop_shortcut > and do_not_create_quick_launch_shortcut Done. https://codereview.chromium.org/12316097/diff/10001/chrome/installer/util/mas... chrome/installer/util/master_preferences_unittest.cc:434: TEST_F(MasterPreferencesTest, On 2013/03/06 20:20:32, gab wrote: > "create_all_shortcuts" is an old preference that has no other impact than > enforcing "do_not_create_quick_launch_shortcut" and > "do_not_create_desktop_shortcut" when explicitly set to false. > > I don't think this test is necessary (especially after addressing the other > comments). That's right, removed.
LGTM w/ tweak below. Once this is in we should ask QA to verify these preferences work as intended and try to merge this in M26 beta (possibly stable if there is enough time left for M25 on stable for it to be worth it). Thanks! Gab https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/12316097/diff/10001/chrome/installer/setup/in... chrome/installer/setup/install.cc:451: start_menu_properties.set_pin_to_taskbar(!do_not_create_taskbar_shortcut); On 2013/03/06 21:21:33, Joao da Silva wrote: > On 2013/03/06 20:20:32, gab wrote: > > Shouldn't set this bit if (!do_not_create_taskbar_shortcut). > > Done (this was just setting it to false in that case, but an explicit if is > clearer) Oh my bad, I had missed this change (had assumed the diff was just an indent plus the if above...). I also prefer it in an explicit if above FWIW anyways. https://codereview.chromium.org/12316097/diff/14004/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/12316097/diff/14004/chrome/installer/setup/in... chrome/installer/setup/install.cc:450: if (!do_not_create_taskbar_shortcut) Please nest this condition in the if above it, i.e.: if (!do_not_create_taskbar_shortcut && (shortcut_operation == ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS || shortcut_operation == ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL)) {
https://codereview.chromium.org/12316097/diff/14004/chrome/installer/setup/in... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/12316097/diff/14004/chrome/installer/setup/in... chrome/installer/setup/install.cc:450: if (!do_not_create_taskbar_shortcut) On 2013/03/06 23:18:16, gab wrote: > Please nest this condition in the if above it, i.e.: > > if (!do_not_create_taskbar_shortcut && > (shortcut_operation == ShellUtil::SHELL_SHORTCUT_CREATE_ALWAYS || > shortcut_operation == > ShellUtil::SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL)) { Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/12316097/21001
Message was sent while issue was closed.
Change committed as 186681 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
