Adapt Chrome OS related part of ChildAccountService.

Adapt Chrome OS related part of ChildAccountService.

Follow up for https://codereview.chromium.org/783083002/

BUG=372381
TBR=ygorshenin@chromium.org

Committed: https://crrev.com/0859fcb9037aff79aab67018fb3db333bf22ebef
Cr-Commit-Position: refs/heads/master@{#307707}

[merkulova, 2014-12-08 11:47:34]

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

[merkulova, 2014-12-08 11:49:13]

merkulova@chromium.org changed reviewers:
+ ygorshenin@chromium.org

[merkulova, 2014-12-08 11:49:56]

Fix for tests. Follow up for https://codereview.chromium.org/783083002/

[merkulova, 2014-12-08 12:52:34]

Uncommented.

[Marc Treib, 2014-12-08 12:59:44]

On 2014/12/08 12:52:34, merkulova wrote:
> Uncommented.

LGTM!

nit: Please spell out ChildAccountService in the description, and maybe also add
a link to https://codereview.chromium.org/783083002/ ?

[Marc Treib, 2014-12-08 16:59:34]

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/mock_user_manager.cc (right):

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/mock_user_manager.cc:112:
profile->set_profile_name(email);
You probably want to add |profile| to |profile_list_|?

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/mock_user_manager.cc:124:
profile->set_profile_name(email);
Here too.

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
File chrome/browser/supervised_user/child_accounts/child_account_service.cc
(right):

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
chrome/browser/supervised_user/child_accounts/child_account_service.cc:302: }
else if (user->GetType() == user_manager::USER_TYPE_REGULAR_SUPERVISED ||
Hm, I don't think we should check the user type here. After all, if we're trying
to set "isChild" to true for a different type of user, then something is wrong,
and we'd like to hit the check in user.cc. Or am I missing something?

[merkulova, 2014-12-10 11:55:45]

Marc, PTAL

Removed DCHECK as it actually fails on starting browser with existing users:
user is created but profile is not completely initiated. So we get NULL. It's ok
as on user sign-in we check child status and set it if necessary.

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/mock_user_manager.cc (right):

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/mock_user_manager.cc:112:
profile->set_profile_name(email);
On 2014/12/08 16:59:34, Marc Treib wrote:
> You probably want to add |profile| to |profile_list_|?

Done.

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/mock_user_manager.cc:124:
profile->set_profile_name(email);
On 2014/12/08 16:59:34, Marc Treib wrote:
> Here too.

Done.

[Marc Treib, 2014-12-10 12:42:36]

Still LGTM, modulo two comments below.

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
File chrome/browser/supervised_user/child_accounts/child_account_service.cc
(right):

https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
chrome/browser/supervised_user/child_accounts/child_account_service.cc:302: }
else if (user->GetType() == user_manager::USER_TYPE_REGULAR_SUPERVISED ||
On 2014/12/08 16:59:34, Marc Treib wrote:
> Hm, I don't think we should check the user type here. After all, if we're
trying
> to set "isChild" to true for a different type of user, then something is
wrong,
> and we'd like to hit the check in user.cc. Or am I missing something?

This one's still not addressed.

https://codereview.chromium.org/785893003/diff/140001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/785893003/diff/140001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:845: user_profile != NULL
&&
Maybe add a comment explaining when user_profile can be null?

[Marc Treib, 2014-12-10 12:43:09]

Ah, also: Please update the description.

[merkulova, 2014-12-10 13:08:17]

On 2014/12/10 12:42:36, Marc Treib wrote:
> Still LGTM, modulo two comments below.
> 
>
https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
> File chrome/browser/supervised_user/child_accounts/child_account_service.cc
> (right):
> 
>
https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
> chrome/browser/supervised_user/child_accounts/child_account_service.cc:302: }
> else if (user->GetType() == user_manager::USER_TYPE_REGULAR_SUPERVISED ||
> On 2014/12/08 16:59:34, Marc Treib wrote:
> > Hm, I don't think we should check the user type here. After all, if we're
> trying
> > to set "isChild" to true for a different type of user, then something is
> wrong,
> > and we'd like to hit the check in user.cc. Or am I missing something?
> 
> This one's still not addressed.

Is ChildAccountService initiated for regular accounts only? If not then we need
that additional check. As we run PropagateChildStatusToUser with false argument
as well.
> 
>
https://codereview.chromium.org/785893003/diff/140001/chrome/browser/ui/ash/s...
> File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> 
>
https://codereview.chromium.org/785893003/diff/140001/chrome/browser/ui/ash/s...
> chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:845: user_profile !=
NULL
> &&
> Maybe add a comment explaining when user_profile can be null?

Done

[Marc Treib, 2014-12-10 13:17:14]

On 2014/12/10 13:08:17, merkulova wrote:
> On 2014/12/10 12:42:36, Marc Treib wrote:
> > Still LGTM, modulo two comments below.
> > 
> >
>
https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
> > File chrome/browser/supervised_user/child_accounts/child_account_service.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/785893003/diff/80001/chrome/browser/supervise...
> > chrome/browser/supervised_user/child_accounts/child_account_service.cc:302:
}
> > else if (user->GetType() == user_manager::USER_TYPE_REGULAR_SUPERVISED ||
> > On 2014/12/08 16:59:34, Marc Treib wrote:
> > > Hm, I don't think we should check the user type here. After all, if we're
> > trying
> > > to set "isChild" to true for a different type of user, then something is
> > wrong,
> > > and we'd like to hit the check in user.cc. Or am I missing something?
> > 
> > This one's still not addressed.
> 
> Is ChildAccountService initiated for regular accounts only? If not then we
need
> that additional check. As we run PropagateChildStatusToUser with false
argument
> as well.

It's inited for all users afaik, but the check in user.cc now only triggers if
we try setting it to true, and that should never happen (right?).

> > 
> >
>
https://codereview.chromium.org/785893003/diff/140001/chrome/browser/ui/ash/s...
> > File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> > 
> >
>
https://codereview.chromium.org/785893003/diff/140001/chrome/browser/ui/ash/s...
> > chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:845: user_profile !=
> NULL
> > &&
> > Maybe add a comment explaining when user_profile can be null?
> 
> Done

[merkulova, 2014-12-10 13:21:57]

> > >
chrome/browser/supervised_user/child_accounts/child_account_service.cc:302:
> }
> > > else if (user->GetType() == user_manager::USER_TYPE_REGULAR_SUPERVISED ||
> > > On 2014/12/08 16:59:34, Marc Treib wrote:
> > > > Hm, I don't think we should check the user type here. After all, if
we're
> > > trying
> > > > to set "isChild" to true for a different type of user, then something is
> > > wrong,
> > > > and we'd like to hit the check in user.cc. Or am I missing something?
> > > 
> > > This one's still not addressed.
> > 
> > Is ChildAccountService initiated for regular accounts only? If not then we
> need
> > that additional check. As we run PropagateChildStatusToUser with false
> argument
> > as well.
> 
> It's inited for all users afaik, but the check in user.cc now only triggers if
> we try setting it to true, and that should never happen (right?).
> 
Makes sense. Do you prefer no-action ignored function call to additional user
type check?

[Marc Treib, 2014-12-10 13:30:38]

On 2014/12/10 13:21:57, merkulova wrote:
> > > >
> chrome/browser/supervised_user/child_accounts/child_account_service.cc:302:
> > }
> > > > else if (user->GetType() == user_manager::USER_TYPE_REGULAR_SUPERVISED
||
> > > > On 2014/12/08 16:59:34, Marc Treib wrote:
> > > > > Hm, I don't think we should check the user type here. After all, if
> we're
> > > > trying
> > > > > to set "isChild" to true for a different type of user, then something
is
> > > > wrong,
> > > > > and we'd like to hit the check in user.cc. Or am I missing something?
> > > > 
> > > > This one's still not addressed.
> > > 
> > > Is ChildAccountService initiated for regular accounts only? If not then we
> > need
> > > that additional check. As we run PropagateChildStatusToUser with false
> > argument
> > > as well.
> > 
> > It's inited for all users afaik, but the check in user.cc now only triggers
if
> > we try setting it to true, and that should never happen (right?).
> > 
> Makes sense. Do you prefer no-action ignored function call to additional user
> type check?

Yes, for two reasons:
1) The CAService shouldn't have to know about CrOS internals, and
2) if PropagateChildStatusToUser(true) were ever called for a non-RegularUser,
then we'd like to hit the DCHECK, so we'll know something's wrong.

[ygorshenin1, 2014-12-10 14:26:23]

lgtm

[merkulova, 2014-12-10 14:26:41]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-12-10 14:27:06]

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

[commit-bot: I haz the power, 2014-12-10 15:57:51]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-12-10 15:58:52]

Patchset 12 (id:??) landed as
https://crrev.com/0859fcb9037aff79aab67018fb3db333bf22ebef
Cr-Commit-Position: refs/heads/master@{#307707}