Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(196)

Issue 24360013: Remove supervised users from the app list profile selector. (Closed)

Created:
7 years, 3 months ago by calamity
Modified:
7 years, 2 months ago
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Remove supervised users from the app list profile selector. This CL filters out managed users from the app list profile selector. Users can still try to switch to the managed user from the app launcher settings. This will result in a no op. BUG=293522 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224984

Patch Set 1 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -0 lines) Patch
M chrome/browser/ui/app_list/app_list_service_impl.cc View 1 chunk +7 lines, -0 lines 2 comments Download
M chrome/browser/ui/app_list/app_list_view_delegate.cc View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
calamity
7 years, 3 months ago (2013-09-24 04:02:03 UTC) #1
tapted
lgtm
7 years, 3 months ago (2013-09-24 04:47:37 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/24360013/6001
7 years, 3 months ago (2013-09-24 09:35:29 UTC) #3
commit-bot: I haz the power
Change committed as 224984
7 years, 3 months ago (2013-09-24 12:43:53 UTC) #4
koz (OOO until 15th September)
https://chromiumcodereview.appspot.com/24360013/diff/6001/chrome/browser/ui/app_list/app_list_service_impl.cc File chrome/browser/ui/app_list/app_list_service_impl.cc (right): https://chromiumcodereview.appspot.com/24360013/diff/6001/chrome/browser/ui/app_list/app_list_service_impl.cc#newcode151 chrome/browser/ui/app_list/app_list_service_impl.cc:151: // Ensure we don't set the pref to a ...
7 years, 2 months ago (2013-09-25 23:35:58 UTC) #5
tapted
https://chromiumcodereview.appspot.com/24360013/diff/6001/chrome/browser/ui/app_list/app_list_service_impl.cc File chrome/browser/ui/app_list/app_list_service_impl.cc (right): https://chromiumcodereview.appspot.com/24360013/diff/6001/chrome/browser/ui/app_list/app_list_service_impl.cc#newcode151 chrome/browser/ui/app_list/app_list_service_impl.cc:151: // Ensure we don't set the pref to a ...
7 years, 2 months ago (2013-09-26 03:52:50 UTC) #6
koz (OOO until 15th September)
7 years, 2 months ago (2013-09-26 05:29:07 UTC) #7
Message was sent while issue was closed.
On 2013/09/26 03:52:50, tapted wrote:
>
https://chromiumcodereview.appspot.com/24360013/diff/6001/chrome/browser/ui/a...
> File chrome/browser/ui/app_list/app_list_service_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/24360013/diff/6001/chrome/browser/ui/a...
> chrome/browser/ui/app_list/app_list_service_impl.cc:151: // Ensure we don't
set
> the pref to a managed user's profile path.
> On 2013/09/25 23:35:58, koz wrote:
> > Is this ever expected to be hit? If not it should be removed. We don't do
> other
> > consistency checks like ensuring that the path exists as it's the caller's
> > responsibility to ensure such things.
> 
> Calamity can confirm, but I'm pretty sure this is the path the Settings App
goes
> through using the button there. There's no filtering for supervised users in
> that list, so if someone switches to a supervised user with the settings app,
it
> will get permanently wedged in a state where it can't show the app list (or
> settings again), because of the check in AppListController::ShowForProfile.
> 
> This is a slight improvement, although not the best. But the Apps story for
> supervised users is broken in a few ways at the moment.

Ah, that is unfortunate. Ok, thanks for the info! I'll add a TODO for fixing it.
(Oh, I also see that this was mentioned in the change description. Sorry for the
noise!)

Powered by Google App Engine
This is Rietveld 408576698