New user type introduced. Combines regular and supervised features.

components/user_manager/user.cc

Index: components/user_manager/user.cc
diff --git a/components/user_manager/user.cc b/components/user_manager/user.cc
index 664aa4ba5041bfb84ed780decfb0e281336b1137..795456836101e99f16dda748c60a3cacded453cf 100644
--- a/components/user_manager/user.cc
+++ b/components/user_manager/user.cc
@@ -28,14 +28,32 @@ std::string GetUserName(const std::string& email) {
 
 }  // namespace
 
-bool User::IsSupervised() const {
-  return false;
-}
-
 void User::SetIsSupervised(bool is_supervised) {

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

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/12 10:01:01]

Done.

   VLOG(1) << "Ignoring SetIsSupervised call with param " << is_supervised;
+  NOTREACHED() << "Calling SetIsSupervised for base User class.";
+}
+
+// static
+bool User::TypeIsRegular(UserType user_type) {
+  return user_type == USER_TYPE_REGULAR ||
+         user_type == USER_TYPE_REGULAR_SUPERVISED;
+}
+
+// static
+bool User::TypeIsSupervised(UserType user_type) {

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

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 19:42:07]

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/12 10:01:01]

Done.

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

Done.

+  return user_type == USER_TYPE_SUPERVISED ||
+         user_type == USER_TYPE_REGULAR_SUPERVISED;
+}
+
+bool User::IsRegular() const {

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

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.

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

Why would you prefer that approach? That would lead to a bunch of
IsSupervised/IsRegular methods scattered around User-classes hierarchy. 
Done

[bartfab (slow), 2014/11/12 16:20:25]

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.

+  return TypeIsRegular(GetType());
 }
 
+bool User::IsSupervised() const {

[bartfab (slow), 2014/11/11 19:42:06]

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/12 10:01:01]

Done.

+  return TypeIsSupervised(GetType());
+}
+
+// Also used for regular supervised users.
 class RegularUser : public User {
  public:
   explicit RegularUser(const std::string& email);
@@ -44,13 +62,7 @@ class RegularUser : public User {
   // Overridden from User:
   virtual UserType GetType() const override;
   virtual bool CanSyncImage() const override;
-  virtual void SetIsSupervised(bool is_supervised) override {
-    VLOG(1) << "Setting user is supervised to " << is_supervised;
-    is_supervised_ = is_supervised;
-  }
-  virtual bool IsSupervised() const override {

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

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.

[merkulova, 2014/11/12 10:01:01]

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).

-    return is_supervised_;
-  }
+  virtual void SetIsSupervised(bool is_supervised) override;
 
  private:
   bool is_supervised_;
@@ -89,7 +101,6 @@ class SupervisedUser : public User {
 
   // Overridden from User:
   virtual UserType GetType() const override;
-  virtual bool IsSupervised() const override;
   virtual std::string display_email() const override;
 
  private:
@@ -252,13 +263,19 @@ RegularUser::~RegularUser() {
 }
 
 UserType RegularUser::GetType() const {
-  return user_manager::USER_TYPE_REGULAR;
+  return is_supervised_ ? user_manager::USER_TYPE_REGULAR_SUPERVISED :
+                          user_manager::USER_TYPE_REGULAR;
 }
 
 bool RegularUser::CanSyncImage() const {
   return true;
 }
 
+void RegularUser::SetIsSupervised(bool is_supervised) {
+  VLOG(1) << "Setting user is supervised to " << is_supervised;
+  is_supervised_ = is_supervised;
+}
+
 GuestUser::GuestUser() : User(chromeos::login::kGuestUserName) {
   set_display_email(std::string());
 }
@@ -297,10 +314,6 @@ std::string SupervisedUser::display_email() const {
   return base::UTF16ToUTF8(display_name());
 }
 
-bool SupervisedUser::IsSupervised() const {
-  return true;
-}
-
 RetailModeUser::RetailModeUser() : User(chromeos::login::kRetailModeUserName) {
   set_display_email(std::string());
 }
@@ -323,9 +336,10 @@ UserType PublicAccountUser::GetType() const {
 }
 
 bool User::has_gaia_account() const {
-  COMPILE_ASSERT(user_manager::NUM_USER_TYPES == 6, num_user_types_unexpected);
+  COMPILE_ASSERT(user_manager::NUM_USER_TYPES == 7, num_user_types_unexpected);
   switch (GetType()) {
     case user_manager::USER_TYPE_REGULAR:
+    case user_manager::USER_TYPE_REGULAR_SUPERVISED:
       return true;
     case user_manager::USER_TYPE_GUEST:
     case user_manager::USER_TYPE_RETAIL_MODE: