ash: Add checks for supervised users.

ash: Add checks for supervised users.

Propagate the user's supervised state to the ash system
so that it becomes available for views

https://chromereviews.googleplex.com/79527013/

BUG=394417
Committed: https://crrev.com/8616ac4cf7bd77f53dc001eb9d8d61c6c975eaaa
Cr-Commit-Position: refs/heads/master@{#294821}

Committed: https://crrev.com/03a72b3faa290ad25a356f247e2d7907b3dfe1af
Cr-Commit-Position: refs/heads/master@{#296160}

[merkulova, 2014-09-10 10:56:54]

merkulova@chromium.org changed reviewers:
+ antrim@chromium.org, bauerb@chromium.org, derat@chromium.org,
jennyz@chromium.org, treib@chromium.com

[merkulova, 2014-09-10 10:58:01]

merkulova@chromium.org changed reviewers:
+ treib@chromium.org
- treib@chromium.com

[merkulova, 2014-09-10 10:58:02]

antrim@ for user_manager/ files review
derat@  for chrome/browser/ui/ash/ files review
jennyz@ for ash/system/ files review

Sorry for spam

[Bernhard Bauer, 2014-09-10 11:26:05]

I don't think my review matters here, but LGTM.

[Marc Treib, 2014-09-10 11:41:15]

I also don't own any of this, but lgtm.

[Daniel Erat, 2014-09-10 15:52:41]

lgtm for c/b/ui/ash

but please write a better description for this change (via the "Edit Issue"
link). changelist descriptions should describe what's affected (in this case,
just ash, i think) and give enough detail that someone who doesn't know anything
about your feature can figure out if your change is relevant when they're
looking through a commit log to find possible causes of a regression. for
example:

"ash: Add checks for supervised users.

Propagate the user's supervised state to the ash system
so that etc." [explain why this is being done]

[jennyz, 2014-09-10 18:13:10]

lgtm, agree with derat, please up the cl description.

[Daniel Erat, 2014-09-11 16:09:39]

thanks, the updated description looks fine to me (i updated it a bit more so the
first line is set correctly)

[Denis Kuznetsov (DE-MUC), 2014-09-11 16:20:54]

lgtm

[merkulova, 2014-09-12 09:34:25]

The CQ bit was checked by merkulova@chromium.org

[merkulova, 2014-09-12 09:34:25]

The CQ bit was unchecked by merkulova@chromium.org

[merkulova, 2014-09-12 09:34:26]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-12 09:50:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561713002/1

[commit-bot: I haz the power, 2014-09-12 10:07:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 10:07:55]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/60088)

[Marc Treib, 2014-09-15 08:16:22]

https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/tray_supervised_user.cc (right):

https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/tray_supervised_user.cc:53: if (!(status !=
ash::user::LOGGED_IN_SUPERVISED ||
Should this be "==" instead of "!="?

[Marc Treib, 2014-09-15 08:20:51]

https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
File components/user_manager/user.cc (right):

https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
components/user_manager/user.cc:36: VLOG(1) << "Setting user is supervised to "
<< is_supervised;
I just noticed: I think this should say something different to the RegularUser
implementation, and probably be at least a LOG(WARNING)? This shouldn't ever be
called, after all.

[merkulova, 2014-09-15 09:38:50]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-15 09:39:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561713002/60001

[commit-bot: I haz the power, 2014-09-15 09:58:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 09:58:06]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Denis Kuznetsov (DE-MUC), 2014-09-15 11:33:36]

https://chromiumcodereview.appspot.com/561713002/diff/60001/ash/test/test_sys...
File ash/test/test_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/561713002/diff/60001/ash/test/test_sys...
ash/test/test_system_tray_delegate.cc:13: 
No need for empty line here.

[merkulova, 2014-09-15 12:34:22]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-15 12:35:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561713002/80001

[commit-bot: I haz the power, 2014-09-15 14:31:27]

Committed patchset #5 (id:80001) as 3cb90f563777c97b42547152259a2719e6e2af87

[commit-bot: I haz the power, 2014-09-15 15:08:36]

Patchset 5 (id:??) landed as
https://crrev.com/8616ac4cf7bd77f53dc001eb9d8d61c6c975eaaa
Cr-Commit-Position: refs/heads/master@{#294821}

[Ken Rockot(use gerrit already), 2014-09-15 23:16:41]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/575653002/ by rockot@chromium.org.

The reason for reverting is: This is breaking
NotificationApiTest.TestUserGesture, which assumes it starts with an empty
notification list.

Somehow this is causing the browser test to be in a state where there is already
a notification in the list regarding a supervised user..

[Nikita (slow), 2014-09-17 18:25:25]

nkostylev@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Nikita (slow), 2014-09-17 18:25:26]

https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
File components/user_manager/user.cc (right):

https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
components/user_manager/user.cc:245: RegularUser::RegularUser(const std::string&
email) : User(email) {
You need to initialize is_supervised_(false) in ctor.

[merkulova, 2014-09-18 08:51:30]

https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
File ash/system/chromeos/supervised/tray_supervised_user.cc (right):

https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
ash/system/chromeos/supervised/tray_supervised_user.cc:53: if (!(status !=
ash::user::LOGGED_IN_SUPERVISED ||
On 2014/09/15 08:16:21, Marc Treib wrote:
> Should this be "==" instead of "!="?

Done.

https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
File components/user_manager/user.cc (right):

https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
components/user_manager/user.cc:36: VLOG(1) << "Setting user is supervised to "
<< is_supervised;
On 2014/09/15 08:20:51, Marc Treib wrote:
> I just noticed: I think this should say something different to the RegularUser
> implementation, and probably be at least a LOG(WARNING)? This shouldn't ever
be
> called, after all.

Done.

https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
File components/user_manager/user.cc (right):

https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
components/user_manager/user.cc:245: RegularUser::RegularUser(const std::string&
email) : User(email) {
On 2014/09/17 18:25:26, Nikita Kostylev wrote:
> You need to initialize is_supervised_(false) in ctor.

Done.

[Nikita (slow), 2014-09-22 13:17:53]

On 2014/09/18 08:51:30, merkulova wrote:
>
https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
> File ash/system/chromeos/supervised/tray_supervised_user.cc (right):
> 
>
https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
> ash/system/chromeos/supervised/tray_supervised_user.cc:53: if (!(status !=
> ash::user::LOGGED_IN_SUPERVISED ||
> On 2014/09/15 08:16:21, Marc Treib wrote:
> > Should this be "==" instead of "!="?
> 
> Done.
> 
>
https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
> File components/user_manager/user.cc (right):
> 
>
https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
> components/user_manager/user.cc:36: VLOG(1) << "Setting user is supervised to
"
> << is_supervised;
> On 2014/09/15 08:20:51, Marc Treib wrote:
> > I just noticed: I think this should say something different to the
RegularUser
> > implementation, and probably be at least a LOG(WARNING)? This shouldn't ever
> be
> > called, after all.
> 
> Done.
> 
>
https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
> File components/user_manager/user.cc (right):
> 
>
https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
> components/user_manager/user.cc:245: RegularUser::RegularUser(const
std::string&
> email) : User(email) {
> On 2014/09/17 18:25:26, Nikita Kostylev wrote:
> > You need to initialize is_supervised_(false) in ctor.
> 
> Done.

Is this the only thing that got changed? What was the root cause of revert?

[Marc Treib, 2014-09-22 13:21:50]

On 2014/09/22 13:17:53, Nikita Kostylev wrote:
> On 2014/09/18 08:51:30, merkulova wrote:
> >
>
https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
> > File ash/system/chromeos/supervised/tray_supervised_user.cc (right):
> > 
> >
>
https://codereview.chromium.org/561713002/diff/20001/ash/system/chromeos/supe...
> > ash/system/chromeos/supervised/tray_supervised_user.cc:53: if (!(status !=
> > ash::user::LOGGED_IN_SUPERVISED ||
> > On 2014/09/15 08:16:21, Marc Treib wrote:
> > > Should this be "==" instead of "!="?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
> > File components/user_manager/user.cc (right):
> > 
> >
>
https://codereview.chromium.org/561713002/diff/20001/components/user_manager/...
> > components/user_manager/user.cc:36: VLOG(1) << "Setting user is supervised
to
> "
> > << is_supervised;
> > On 2014/09/15 08:20:51, Marc Treib wrote:
> > > I just noticed: I think this should say something different to the
> RegularUser
> > > implementation, and probably be at least a LOG(WARNING)? This shouldn't
ever
> > be
> > > called, after all.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
> > File components/user_manager/user.cc (right):
> > 
> >
>
https://codereview.chromium.org/561713002/diff/80001/components/user_manager/...
> > components/user_manager/user.cc:245: RegularUser::RegularUser(const
> std::string&
> > email) : User(email) {
> > On 2014/09/17 18:25:26, Nikita Kostylev wrote:
> > > You need to initialize is_supervised_(false) in ctor.
> > 
> > Done.
> 
> Is this the only thing that got changed? What was the root cause of revert?

I think this may well have been the root cause: If a regular user was flagged as
supervised, that might have caused the SU notification to appear when it
shouldn't.

[Nikita (slow), 2014-09-22 13:40:15]

lgtm

[merkulova, 2014-09-23 08:10:38]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-09-23 08:11:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/561713002/100001

[commit-bot: I haz the power, 2014-09-23 08:51:16]

Committed patchset #6 (id:100001) as ffc70a906805df68fceeb071eec2dae38202b964

[commit-bot: I haz the power, 2014-09-23 08:51:56]

Patchset 6 (id:??) landed as
https://crrev.com/03a72b3faa290ad25a356f247e2d7907b3dfe1af
Cr-Commit-Position: refs/heads/master@{#296160}