Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
542843002:
[Win] Don't show the add/remove desktop shortcut for single users. (Closed)
|
Created:
6 years, 3 months ago by noms (inactive) Modified:
6 years, 3 months ago Reviewers:
Dan Beam CC:
chromium-reviews, dbeam+watch-options_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Win] Don't show the add/remove desktop shortcut for single users.
If there's a single user in chrome://settings, you can navigate to
chrome://settings/manageProfile to edit it (even though the UI doesn't
always let you). In this case, however, we shouldn't display the add/remove
desktop shortcut button in the manageProfile overlay (since the shortcut for
single users is a non-profile generic Chrome shortcut)
BUG=409735
TEST=Start Chrome. Ensure you only have one profile.
Go to chrome://settings/manageProfile. There shouldn't be an add/remove
shortcut button.
Committed: https://crrev.com/11ae275b8b1b06b5bd2f85760a050fc9b6c58c99
Cr-Commit-Position: refs/heads/master@{#293804}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : review comments #
Total comments: 4
Patch Set 3 : move code to a better place #Messages
Total messages: 15 (4 generated)
Patchset #1 (id:1) has been deleted
noms@chromium.org changed reviewers: + dbeam@chromium.org
Cleaning up the manageProfile overlay a little, since we've enabled always showing the Users table. https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:505: if (switches::IsNewAvatarMenu() && cache.GetNumberOfProfiles() > 1) { Question for you: Even in the pre-mirror world, if you had one user you could still manually navigate to chrome://settings/manageProfile, in which case you would see this add/remove desktop shortcut button. Do you think I should hide it in the overlay regardless of whether -new-avatar-menu is turned off?
https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:498: void ManageProfileHandler::OnHasProfileShortcuts(bool has_shortcuts) { i'm slightly more partial to CHECK() than an if -> return if it means we're doing less work. when is this allowed to be called when !IsNewAvatarMenu and GetNumberOfProfiles() <= 1? https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:500: nit: if the code stays as it: if (!switches::IsNewAvatarMenu()) return; const ProfileInfoCache& cache = g_browser_process->profile_manager()->GetProfileInfoCache(); if (cache.GetNumberOfProfiles() <= 1) return; const base::FundamentalValue has_shortcuts_value(has_shortcuts); web_ui()->CallJavascriptFunction( "ManageProfileOverlay.receiveHasProfileShortcuts", has_shortcuts_value); https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:505: if (switches::IsNewAvatarMenu() && cache.GetNumberOfProfiles() > 1) { On 2014/09/04 21:24:13, Monica Dinculescu wrote: > Question for you: Even in the pre-mirror world, if you had one user you could > still manually navigate to chrome://settings/manageProfile, in which case you > would see this add/remove desktop shortcut button. Do you think I should hide it > in the overlay regardless of whether -new-avatar-menu is turned off? i don't see it on Linux. you should hide irrelevant UI, yes.
https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:498: void ManageProfileHandler::OnHasProfileShortcuts(bool has_shortcuts) { You can manually navigate to chrome://settings/manageProfille without the flag on, and with only one user, so I don't think a check is going to work. On 2014/09/04 22:59:09, Dan Beam wrote: > i'm slightly more partial to CHECK() than an if -> return if it means we're > doing less work. when is this allowed to be called when !IsNewAvatarMenu and > GetNumberOfProfiles() <= 1? https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:500: On 2014/09/04 22:59:09, Dan Beam wrote: > nit: if the code stays as it: > > if (!switches::IsNewAvatarMenu()) > return; > > const ProfileInfoCache& cache = > g_browser_process->profile_manager()->GetProfileInfoCache(); > if (cache.GetNumberOfProfiles() <= 1) > return; > > const base::FundamentalValue has_shortcuts_value(has_shortcuts); > web_ui()->CallJavascriptFunction( > "ManageProfileOverlay.receiveHasProfileShortcuts", has_shortcuts_value); Done. https://codereview.chromium.org/542843002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:505: if (switches::IsNewAvatarMenu() && cache.GetNumberOfProfiles() > 1) { This is just for Windows (Linux doesn't have desktop shortcuts). I've hidden the UI in all cases. On 2014/09/04 22:59:09, Dan Beam wrote: > On 2014/09/04 21:24:13, Monica Dinculescu wrote: > > Question for you: Even in the pre-mirror world, if you had one user you could > > still manually navigate to chrome://settings/manageProfile, in which case you > > would see this add/remove desktop shortcut button. Do you think I should hide > it > > in the overlay regardless of whether -new-avatar-menu is turned off? > > i don't see it on Linux. you should hide irrelevant UI, yes.
https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:499: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); what happened to the flag check?
https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:499: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Maybe I misunderstood your reply, but I thought you said I should always hide the useless UI. Without the flag, with one profile, while you can't press the "Edit" button in the list to edit the profile (because there is no list), you can still navigate to chrome://settings/manageProfile, and would see the desktop shortcut button. On 2014/09/05 19:19:15, Dan Beam wrote: > what happened to the flag check?
https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:499: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2014/09/05 19:50:37, Monica Dinculescu wrote: > Maybe I misunderstood your reply, but I thought you said I should always hide > the useless UI. where is the code that hides the useless UI? > > Without the flag, with one profile, while you can't press the "Edit" button in > the list to edit the profile (because there is no list), you can still navigate > to chrome://settings/manageProfile, and would see the desktop shortcut button. > > On 2014/09/05 19:19:15, Dan Beam wrote: > > what happened to the flag check? >
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/options/manage_profile_handler.cc (right): https://codereview.chromium.org/542843002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/options/manage_profile_handler.cc:499: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Initially, in manage_profile_overlay.js, in didShowPage, we have $('remove-shortcut-button').hidden = true; $('add-shortcut-button').hidden = true; Then 'requestHasProfileShortcuts' is called. I've moved this code to that function, as it was a better place and I missed it the first time around, but basically when OnHasProfileShortcuts returns, it would call the JS 'receiveHasProfileShortcuts', which changes the visibility of the buttons. So if we don't ever call receiveHasProfileShortcuts, the buttons stay in their default state, hidden. On 2014/09/05 22:16:43, Dan Beam wrote: > On 2014/09/05 19:50:37, Monica Dinculescu wrote: > > Maybe I misunderstood your reply, but I thought you said I should always hide > > the useless UI. > > where is the code that hides the useless UI? > > > > > Without the flag, with one profile, while you can't press the "Edit" button in > > the list to edit the profile (because there is no list), you can still > navigate > > to chrome://settings/manageProfile, and would see the desktop shortcut button. > > > > On 2014/09/05 19:19:15, Dan Beam wrote: > > > what happened to the flag check? > > >
lgtm
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/542843002/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as a28db8a8c779cce237d17b6134c8476ade35c2da
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/11ae275b8b1b06b5bd2f85760a050fc9b6c58c99 Cr-Commit-Position: refs/heads/master@{#293804} |
