New user type introduced. Combines regular and supervised features.

New user type introduced. Combines regular and supervised features.
BUG=419009
Inherited from initial CL https://chromereviews.googleplex.com/100407014/

TBR=oshima@chromium.org, achuithb@chromium.org, Finnur@chromium.org, xiyuan@chromium.org, jennyz@chromium.org, bartfab@chromium.org

Committed: https://crrev.com/c3ae44da8c3e504e6a74726837016f9eb54c05e6
Cr-Commit-Position: refs/heads/master@{#304398}

[merkulova, 2014-11-11 11:31:50]

merkulova@chromium.org changed reviewers:
+ bartfab@google.com, derat@chromium.org, dtseng@google.com,
nkostylev@google.com

[merkulova, 2014-11-11 11:34:38]

merkulova@chromium.org changed reviewers:
+ bartfab@chromium.org, dtseng@chromium.org, nkostylev@chromium.org
- bartfab@google.com, dtseng@google.com, nkostylev@google.com

[merkulova, 2014-11-11 11:34:38]

derat@ for 	chrome/browser/ui/ash/system_tray_delegate_chromeos.cc review
dtseng@ for chrome/browser/chromeos/accessibility/accessibility_manager.cc
review
bartfab@ for chrome/browser/chromeos/policy/* review

nkostylev@ for general review

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

https://codereview.chromium.org/718673002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/718673002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:231:
LoginUtils::IsWhitelisted((*it)->email(), NULL) || (*it)->IsRegular();
This has to be !(*it)->IsRegular()

https://codereview.chromium.org/718673002/diff/1/chrome/browser/ui/ash/system...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/718673002/diff/1/chrome/browser/ui/ash/system...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:557: if
(!user_manager::UserManager::Get()->GetActiveUser()->IsRegular())
IsRegular() && !IsSupervised()

Might as well add helper function: IsRegularNotSupervised()

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user.h
File components/user_manager/user.h (right):

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user...
components/user_manager/user.h:100: virtual void SetIsSupervised(bool
is_supervised);
nit: Add comment.

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user...
File components/user_manager/user_type.h (right):

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user...
components/user_manager/user_type.h:29: NUM_USER_TYPES = 7
You need to update UserType in tools/metrics/histograms/histograms.xml

[merkulova, 2014-11-11 15:47:44]

https://codereview.chromium.org/718673002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/718673002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:231:
LoginUtils::IsWhitelisted((*it)->email(), NULL) || (*it)->IsRegular();
On 2014/11/11 12:06:36, Nikita wrote:
> This has to be !(*it)->IsRegular()

Done.

https://codereview.chromium.org/718673002/diff/1/chrome/browser/ui/ash/system...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/718673002/diff/1/chrome/browser/ui/ash/system...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:557: if
(!user_manager::UserManager::Get()->GetActiveUser()->IsRegular())
On 2014/11/11 12:06:36, Nikita wrote:
> IsRegular() && !IsSupervised()
> 
> Might as well add helper function: IsRegularNotSupervised()

As far as we need it once for now, leaving it as combo.

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user.h
File components/user_manager/user.h (right):

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user...
components/user_manager/user.h:100: virtual void SetIsSupervised(bool
is_supervised);
On 2014/11/11 12:06:36, Nikita wrote:
> nit: Add comment.

Done.

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user...
File components/user_manager/user_type.h (right):

https://codereview.chromium.org/718673002/diff/1/components/user_manager/user...
components/user_manager/user_type.h:29: NUM_USER_TYPES = 7
On 2014/11/11 12:06:36, Nikita wrote:
> You need to update UserType in tools/metrics/histograms/histograms.xml

Done.

[Nikita (slow), 2014-11-11 15:51:55]

lgtm

[Daniel Erat, 2014-11-11 16:02:49]

lgtm for c/b/ui with some comments

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:556: // Only regular
users could add other users to current session.
please update this comment to describe what the condition is actually testing
now (e.g. "Only regular, non-supervised users ...")

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:558:
user_manager::UserManager::Get()->GetActiveUser()->IsSupervised()) {
nit: indent this one space less:

  if (!user_manager::... ||
      user_manager::...) {

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

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:43: bool User::TypeIsSupervised(UserType
user_type) {
i don't see this static method getting called anywhere besides
User::IsSupervised(). if you don't have plans to call it from somewhere else,
could you just inline it within IsSupervised()?

[bartfab (slow), 2014-11-11 17:22:58]

Since this is a new user type, why do we not introduce a new subclass of User
along with a new entry in the user type enums?

It would be a lot cleaner to introduce a whole new user type rather than
defining a sub-type of an existing user.

[David Tseng, 2014-11-11 17:41:30]

lgtm for accessibility_manager.cc

[David Tseng, 2014-11-11 17:44:58]

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:485:
user_manager::UserManager::Get()->
nit: simplify this expression to !a && !b

[bartfab (slow), 2014-11-11 19:42:07]

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc
(right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:36:
#include "components/user_manager/user_type.h"
Nit: No longer used.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:36:
#include "components/user_manager/user_type.h"
Nit: No longer used.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:19:
#include "components/user_manager/user_type.h"
Nit: No longer used.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:103: #include
"components/user_manager/user_type.h"
Nit: No longer used.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:557: if
(!user_manager::UserManager::Get()->GetActiveUser()->IsRegular() ||
The old code implemented the exactly same condition in a simpler way.

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

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:51: virtual bool IsSupervised() const override {
This was the right way of doing things. Why did you change it? The idea is that
the methods in |User| return generic defaults which subclasses can override as
required. That way, each subclass bundles the information about its properties.

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

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:31: void User::SetIsSupervised(bool
is_supervised) {
The definition order does not match the declaration order. This should be down
near line 133, with the rest of the |User| methods.

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:43: bool User::TypeIsSupervised(UserType
user_type) {
The definition order does not match the declaration order. This should be down
near line 133, with the rest of the |User| methods.

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:48: bool User::IsRegular() const {
1: This should be implemented in the same way as the original IsSupervised():
|User| has a virtual IsRegular() method that returns |false|. |RegularUser|
overrides it and returns |true| instead. That way, each subclass bundles the
information about its properties.

2: The definition order does not match the declaration order. This should be
down near line 133, with the rest of the |User| methods.

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:52: bool User::IsSupervised() const {
The definition order does not match the declaration order. This should be down
near line 133, with the rest of the |User| methods.

[merkulova, 2014-11-11 20:21:43]

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

[merkulova, 2014-11-11 20:22:05]

+asvitkine@ for histograms.xml review

[Alexei Svitkine (slow), 2014-11-11 20:32:00]

lgtm

[merkulova, 2014-11-12 10:01:02]

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:485:
user_manager::UserManager::Get()->
On 2014/11/11 17:44:57, David Tseng wrote:
> nit: simplify this expression to !a && !b

Done.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc
(right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.cc:36:
#include "components/user_manager/user_type.h"
On 2014/11/11 19:42:06, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:36:
#include "components/user_manager/user_type.h"
On 2014/11/11 19:42:06, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:19:
#include "components/user_manager/user_type.h"
On 2014/11/11 19:42:06, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:103: #include
"components/user_manager/user_type.h"
On 2014/11/11 19:42:06, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:556: // Only regular
users could add other users to current session.
On 2014/11/11 16:02:49, Daniel Erat wrote:
> please update this comment to describe what the condition is actually testing
> now (e.g. "Only regular, non-supervised users ...")

Done.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:557: if
(!user_manager::UserManager::Get()->GetActiveUser()->IsRegular() ||
On 2014/11/11 19:42:06, bartfab wrote:
> The old code implemented the exactly same condition in a simpler way.

Done.

https://codereview.chromium.org/718673002/diff/20001/chrome/browser/ui/ash/sy...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:558:
user_manager::UserManager::Get()->GetActiveUser()->IsSupervised()) {
On 2014/11/11 16:02:49, Daniel Erat wrote:
> nit: indent this one space less:
> 
>   if (!user_manager::... ||
>       user_manager::...) {

Done.

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

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:51: virtual bool IsSupervised() const override {
On 2014/11/11 19:42:07, bartfab wrote:
> This was the right way of doing things. Why did you change it? The idea is
that
> the methods in |User| return generic defaults which subclasses can override as
> required. That way, each subclass bundles the information about its
properties.

Because new user type was introduced. Before there was one user type used for
both true regular users and unicorns. So now all we should do to make decision
is based on user type (GetType call).

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

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:31: void User::SetIsSupervised(bool
is_supervised) {
On 2014/11/11 19:42:07, bartfab wrote:
> The definition order does not match the declaration order. This should be down
> near line 133, with the rest of the |User| methods.

Done.

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:43: bool User::TypeIsSupervised(UserType
user_type) {
On 2014/11/11 16:02:49, Daniel Erat wrote:
> i don't see this static method getting called anywhere besides
> User::IsSupervised(). if you don't have plans to call it from somewhere else,
> could you just inline it within IsSupervised()?

Done.

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:43: bool User::TypeIsSupervised(UserType
user_type) {
On 2014/11/11 19:42:07, bartfab wrote:
> The definition order does not match the declaration order. This should be down
> near line 133, with the rest of the |User| methods.

Done.

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:48: bool User::IsRegular() const {
On 2014/11/11 19:42:07, bartfab wrote:
> 1: This should be implemented in the same way as the original IsSupervised():
> |User| has a virtual IsRegular() method that returns |false|. |RegularUser|
> overrides it and returns |true| instead. That way, each subclass bundles the
> information about its properties.
Why would you prefer that approach? That would lead to a bunch of
IsSupervised/IsRegular methods scattered around User-classes hierarchy. 

> 2: The definition order does not match the declaration order. This should be
> down near line 133, with the rest of the |User| methods.
Done

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:52: bool User::IsSupervised() const {
On 2014/11/11 19:42:06, bartfab wrote:
> The definition order does not match the declaration order. This should be down
> near line 133, with the rest of the |User| methods.

Done.

[Nikita (slow), 2014-11-12 10:55:38]

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
File components/user_manager/user.h (right):

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
components/user_manager/user.h:100: // Returns true if user is regular.
nit: mention that this is true for both 
USER_TYPE_REGULAR and USER_TYPE_REGULAR_SUPERVISED.

[Nikita (slow), 2014-11-12 10:56:33]

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
File components/user_manager/user.h (right):

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
components/user_manager/user.h:100: // Returns true if user is regular.
On 2014/11/12 10:55:38, Nikita wrote:
> nit: mention that this is true for both 
> USER_TYPE_REGULAR and USER_TYPE_REGULAR_SUPERVISED.

One can easily think that these checks are identical:

user->GetType() == user_manager::USER_TYPE_REGULAR
user->IsRegular()

[merkulova, 2014-11-12 11:01:17]

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
File components/user_manager/user.h (right):

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
components/user_manager/user.h:100: // Returns true if user is regular.
On 2014/11/12 10:55:38, Nikita wrote:
> nit: mention that this is true for both 
> USER_TYPE_REGULAR and USER_TYPE_REGULAR_SUPERVISED.

Done.

[bartfab (slow), 2014-11-12 16:20:26]

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

https://codereview.chromium.org/718673002/diff/20001/components/user_manager/...
components/user_manager/user.cc:48: bool User::IsRegular() const {
On 2014/11/12 10:01:02, merkulova wrote:
> On 2014/11/11 19:42:07, bartfab wrote:
> > 1: This should be implemented in the same way as the original
IsSupervised():
> > |User| has a virtual IsRegular() method that returns |false|. |RegularUser|
> > overrides it and returns |true| instead. That way, each subclass bundles the
> > information about its properties.
> Why would you prefer that approach? That would lead to a bunch of
> IsSupervised/IsRegular methods scattered around User-classes hierarchy. 
> 
> > 2: The definition order does not match the declaration order. This should be
> > down near line 133, with the rest of the |User| methods.
> Done

The idea is that you have a class which encapsulates all the knowledge about a
particular type of user. If we were to use global methods like the ones you
propose, we would not need to create subclasses of |User| at all.

Yes, you end up with multiple IsSupervised() / IsRegular() methods. This is by
design. Everyone who is supervised or regular has such a method, making it very
explicit.

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
File components/user_manager/user.h (right):

https://codereview.chromium.org/718673002/diff/40001/components/user_manager/...
components/user_manager/user.h:100: // Returns true if user is regular.
On 2014/11/12 10:56:32, Nikita wrote:
> On 2014/11/12 10:55:38, Nikita wrote:
> > nit: mention that this is true for both 
> > USER_TYPE_REGULAR and USER_TYPE_REGULAR_SUPERVISED.
> 
> One can easily think that these checks are identical:
> 
> user->GetType() == user_manager::USER_TYPE_REGULAR
> user->IsRegular()
> 

This actually bothered me about this CL from the start but I cannot think of a
better term than "regular" that would make things less ambiguous.

[merkulova, 2014-11-13 14:15:35]

On discussion with bartfab@ renaming was introduced.
Nikita, Bartosz, PTAL before I add more reviewers as owners of freshly changed
files.

[Nikita (slow), 2014-11-13 15:51:47]

On 2014/11/13 14:15:35, merkulova wrote:
> On discussion with bartfab@ renaming was introduced.
> Nikita, Bartosz, PTAL before I add more reviewers as owners of freshly changed
> files.

As discussed offline, I propose that we avoid mentioning GAIA but instead use
something more generic.

[Nikita (slow), 2014-11-14 09:30:22]

On 2014/11/13 15:51:47, Nikita wrote:
> On 2014/11/13 14:15:35, merkulova wrote:
> > On discussion with bartfab@ renaming was introduced.
> > Nikita, Bartosz, PTAL before I add more reviewers as owners of freshly
changed
> > files.
> 
> As discussed offline, I propose that we avoid mentioning GAIA but instead use
> something more generic.

FYI, there's already IdentityProvider base class:
https://code.google.com/p/chromium/codesearch#chromium/src/google_apis/gaia/i...

Even though comments mention GAIA it is might be later used for other identity
providers replacing GAIA.

SAML identity providers should not be confused with "primary" identity provider.

[Nikita (slow), 2014-11-14 09:34:41]

On 2014/11/13 14:15:35, merkulova wrote:
> On discussion with bartfab@ renaming was introduced.
> Nikita, Bartosz, PTAL before I add more reviewers as owners of freshly changed
> files.

lgtm, GAIA mentions can be renamed to something more generic once we'll be
adding such support.

Actually mentioning GAIA explicitly maybe even helpful i.e. this is just another
place which needs to be audited when generic identity provider support is being
added.

[Nikita (slow), 2014-11-14 09:34:52]

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

https://codereview.chromium.org/718673002/diff/80001/components/user_manager/...
components/user_manager/user.cc:32: bool User::TypeHasGaia(UserType user_type) {
nit: TypeHasGaiaAccount

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

https://codereview.chromium.org/718673002/diff/80001/components/user_manager/...
components/user_manager/user_manager_base.cc:250: NOTREACHED() << "Switching to
a non-regular user";
nit: update comment?

[merkulova, 2014-11-14 10:09:48]

merkulova@chromium.org changed reviewers:
+ achuith@chromium.org, finnur@chromium.org, jennyz@chromium.org,
oshima@chromium.org, xiyuan@chromium.org

[merkulova, 2014-11-14 10:09:49]

+oshima@ for .../display/* review
+dzhioev@ for drive_first_run_controller review
+achuith@ for autotest_private_api.cc review
+finnur@ for extension_asset_manager_chromeos.cc review
+xiyuan@ for easy_unlock* review
+jennyz@ for system_tray_delegate_chromeos review

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

https://codereview.chromium.org/718673002/diff/80001/components/user_manager/...
components/user_manager/user.cc:32: bool User::TypeHasGaia(UserType user_type) {
On 2014/11/14 09:34:52, Nikita wrote:
> nit: TypeHasGaiaAccount

Done.

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

https://codereview.chromium.org/718673002/diff/80001/components/user_manager/...
components/user_manager/user_manager_base.cc:250: NOTREACHED() << "Switching to
a non-regular user";
On 2014/11/14 09:34:52, Nikita wrote:
> nit: update comment?

Done.

[merkulova, 2014-11-14 10:16:28]

merkulova@chromium.org changed reviewers:
- achuith@chromium.org, finnur@chromium.org, jennyz@chromium.org,
oshima@chromium.org, xiyuan@chromium.org

[merkulova, 2014-11-14 10:16:45]

Moving to TBR

[merkulova, 2014-11-14 13:34:13]

merkulova@chromium.org changed reviewers:
- bartfab@chromium.org

[Nikita (slow), 2014-11-14 13:36:17]

The CQ bit was checked by nkostylev@chromium.org

[Nikita (slow), 2014-11-14 13:36:17]

The CQ bit was unchecked by nkostylev@chromium.org

[merkulova, 2014-11-14 13:36:18]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-11-14 13:36:56]

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

[commit-bot: I haz the power, 2014-11-14 14:56:17]

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

[commit-bot: I haz the power, 2014-11-14 14:56:18]

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

[xiyuan, 2014-11-14 16:19:06]

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

[xiyuan, 2014-11-14 16:19:07]

easy_unlock* LGTM

[Finnur, 2014-11-14 18:46:44]

extension_asset_manager_chromeos.cc LGTM (rubberstamp)

[merkulova, 2014-11-17 08:38:42]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-11-17 08:39:12]

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

[commit-bot: I haz the power, 2014-11-17 09:35:17]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-17 09:35:57]

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

[bartfab (slow), 2014-11-27 12:51:48]

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

[bartfab (slow), 2014-11-27 12:51:51]

Belated lgtm with a couple of nits.

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:486: return
!user_manager::UserManager::Get()->IsLoggedInAsSupervisedUser() &&
Nit: It would be more elegant to do this instead:
const User* user = use user_manager::UserManager::Get()->GetActiveUser();
return !user || !user->IsSupervised();

That way, this code does not need to know all the different supervised user
types.

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/drive_first_run_controller.cc (right):

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/drive_first_run_controller.cc:365: "but not
logged in a regular user.";
Nit: s/a regular/as a gaia/

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:331: // We need to
restore session only for logged in GAIA (regular) users.
Nit: As elsewhere, I think we can drop the "regular."

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1204: // Only
update Easy unlock keys for regular user.
Nit: s/regular/gaia/

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin/merge_session_throttle.cc (right):

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin/merge_session_throttle.cc:169: // This is
not a regular user session, let's remove the throttle
Nit: s/regular/gaia/

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.h
(right):

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/avatar/user_image_manager_impl.h:152: //
Returns true if user with |user_id_| is logged in and has gaia account.
Nit: s/has/has a/

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

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:579: //
opportunity to clean up by removing all regular users except the owner.
Nit: s/regular/gaia/

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_status_collector.cc (right):

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_status_collector.cc:434: // Only users
with gaia accounts (regular) are reported.
Nit: As before, I think we should drop the " (regular)"

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/718673002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:132:
// USER_TYPE_RETAIL_MODE, USER_TYPE_KIOSK_APP, USER_TYPE_GUEST
This is actually out of date. USER_TYPE_KIOSK_APP gets policy from
DeviceLocalAccountPolicyService, just like USER_TYPE_PUBLIC_ACCOUNT.

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

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user.cc:147: UserType type = GetType();
Nit: Const.

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
File components/user_manager/user.h (right):

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user.h:78: // Returns true if user type has gaia
account.
Nit 1: s/user/the user/
Nit 2: s/gaia/a gaia/

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user.h:97: // Allows managing supervised status of the
user. Used for RegularUser.
Nit 1: "Managing" is somewhat overloaded. I think "setting" would be more
neutral.
Nit 2: s/managing supervised status of the user/management of the user's
supervised status/
Nit 3: The reference to the |RegularUser| class here in the header is strange as
that class is only defined as an internal detail in the implementation file. How
about "regular user" instead?

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user.h:100: // Returns true if user has gaia account.
True for users of types
Nit 1: s/user/the user/
Nit 2: s/gaia/a gaia/

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
File components/user_manager/user_manager.h (right):

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user_manager.h:256: // Returns true if we're logged in
as a user with gaia account.
Nit: s/with/with a/

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

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user_manager_base.cc:61: // a user with gaia account
(regular) or an empty string if it was another type
Nit: As before, I am not sure "regular" is actually helpful here.

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user_manager_base.cc:251: "Switching to a user without
gaia account (non-regular one)";
Nit: As before, "non-regular one" is not really helpful. We have so many user
types now that "regular" vs. "non-regular" is pretty murky.

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user_manager_base.cc:620: // a) The user logged into a
regular gaia account while the ephemeral users
Nit: s/into a regular/in with a/ (there is not such thing as a "regular gaia
account")

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user_manager_base.cc:668: // Only regular and supervised
users are allowed to be manually removed.
Nit: "regular" and "supervised" overlap now that we also have "regular
supervised." How about "Only gaia and legacy supervised users"?

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
File components/user_manager/user_manager_base.h (right):

https://codereview.chromium.org/718673002/diff/120001/components/user_manager...
components/user_manager/user_manager_base.h:311: // Sends metrics in response to
a user with gaia account (regular) logging in.
Nit 1: s/gaia/a gaia/
Nit 2: I am not sure the "regular" here makes things clearer. If anything, it
makes it sound like there are different typesof users with gaia accounts and
this method apply to a subset of them (the regular users) only.