Creating supervised users restricted for regular users if they are supervised.

Creating supervised users restricted for regular users if they are supervised.

Initial CL https://chromereviews.googleplex.com/103087013/

BUG=419009
Committed: https://crrev.com/b82b713344a8ecc98a71ae70eabb3ed434763d61
Cr-Commit-Position: refs/heads/master@{#304407}

[merkulova, 2014-11-12 10:46:19]

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

[Nikita (slow), 2014-11-12 11:09:09]

lgtm

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/chrome_user_manager.cc (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/chrome_user_manager.cc:30: if
(user->GetType() == user_manager::USER_TYPE_REGULAR) {
nit: merge with https://codereview.chromium.org/718673002/, 
may use here
if (user->IsRegular() && !user->IsSupervised())

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/chrome_user_manager.h (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/chrome_user_manager.h:34: // Helper method
for sorting out of user list only users that can create SU.
nit: avoid abbreviations such as SU - supervised users.
Same for method name.

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:237: bool
allow_new_user = true;
https://code.google.com/p/chromium/codesearch#search/&q=kAccountsPrefAllowNew...

It seems that we initialize allow_new_user to both true and false depending on
the context.

However cros_settings->GetBoolean() will not initialize it if this value is
absent in settings store.

It seems that initializing this to allow_new_user = true is the right way to
proceed.

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/fake_user_manager.cc (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/fake_user_manager.cc:116: // static
nit: it's not static

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

https://codereview.chromium.org/720723002/diff/20001/components/user_manager/...
components/user_manager/user_manager_base.cc:784:
prefs_user_types->GetIntegerWithoutPathExpansion(*it, &user_type);
nit: Drop this line.

https://codereview.chromium.org/720723002/diff/20001/components/user_manager/...
components/user_manager/user_manager_base.cc:996: SaveUserType(user->email(),
is_supervised ?
nit: run git cl format on your patch.

[merkulova, 2014-11-12 13:19:51]

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/chrome_user_manager.cc (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/chrome_user_manager.cc:30: if
(user->GetType() == user_manager::USER_TYPE_REGULAR) {
On 2014/11/12 11:09:08, Nikita wrote:
> nit: merge with https://codereview.chromium.org/718673002/, 
> may use here
> if (user->IsRegular() && !user->IsSupervised())

Why not simply leave this check?

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/chrome_user_manager.h (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/chrome_user_manager.h:34: // Helper method
for sorting out of user list only users that can create SU.
On 2014/11/12 11:09:08, Nikita wrote:
> nit: avoid abbreviations such as SU - supervised users.
> Same for method name.

Done.

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:237: bool
allow_new_user = true;
On 2014/11/12 11:09:08, Nikita wrote:
>
https://code.google.com/p/chromium/codesearch#search/&q=kAccountsPrefAllowNew...
> 
> It seems that we initialize allow_new_user to both true and false depending on
> the context.
> 
> However cros_settings->GetBoolean() will not initialize it if this value is
> absent in settings store.
> 
> It seems that initializing this to allow_new_user = true is the right way to
> proceed.

Acknowledged.

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/fake_user_manager.cc (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/fake_user_manager.cc:116: // static
On 2014/11/12 11:09:09, Nikita wrote:
> nit: it's not static

Done.

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

https://codereview.chromium.org/720723002/diff/20001/components/user_manager/...
components/user_manager/user_manager_base.cc:784:
prefs_user_types->GetIntegerWithoutPathExpansion(*it, &user_type);
On 2014/11/12 11:09:09, Nikita wrote:
> nit: Drop this line.

Done.

[Nikita (slow), 2014-11-12 13:43:38]

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/chrome_user_manager.cc (right):

https://codereview.chromium.org/720723002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/chrome_user_manager.cc:30: if
(user->GetType() == user_manager::USER_TYPE_REGULAR) {
On 2014/11/12 13:19:51, merkulova wrote:
> On 2014/11/12 11:09:08, Nikita wrote:
> > nit: merge with https://codereview.chromium.org/718673002/, 
> > may use here
> > if (user->IsRegular() && !user->IsSupervised())
> 
> Why not simply leave this check?

I think that this condition (user->IsRegular() && !user->IsSupervised()) is more
self-descriptive.

Looking at (user->GetType() == user_manager::USER_TYPE_REGULAR) one can easily
forget that there's also USER_TYPE_REGULAR_SUPERVISED.

Plus condition that I propose is shorter and doesn't expose enum value which
should be avoid if possible.

[Nikita (slow), 2014-11-12 13:44:39]

lgtm

[merkulova, 2014-11-17 10:26:22]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-11-17 10:26:41]

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

[commit-bot: I haz the power, 2014-11-17 11:06:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-17 11:07:51]

Patchset 4 (id:??) landed as
https://crrev.com/b82b713344a8ecc98a71ae70eabb3ed434763d61
Cr-Commit-Position: refs/heads/master@{#304407}