New user type introduced. Combines regular and supervised features.

components/user_manager/user.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/user_manager/user.h"
 
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
@@ skipping 10 matching lines @@
 std::string GetUserName(const std::string& email) {
   std::string::size_type i = email.find('@');
   if (i == 0 || i == std::string::npos) {
     return email;
   }
   return email.substr(0, i);
 }
 
 }  // 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.";
 }
 
-void User::SetIsSupervised(bool is_supervised) {
-  VLOG(1) << "Ignoring SetIsSupervised call with param " << is_supervised;
+// 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);
   virtual ~RegularUser();
 
   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(RegularUser);
 };
 
 class GuestUser : public User {
  public:
   GuestUser();
@@ skipping 18 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(KioskAppUser);
 };
 
 class SupervisedUser : public User {
  public:
   explicit SupervisedUser(const std::string& username);
   virtual ~SupervisedUser();
 
   // Overridden from User:
   virtual UserType GetType() const override;
-  virtual bool IsSupervised() const override;
   virtual std::string display_email() const override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SupervisedUser);
 };
 
 class RetailModeUser : public User {
  public:
   RetailModeUser();
   virtual ~RetailModeUser();
@@ skipping 142 matching lines @@
 RegularUser::RegularUser(const std::string& email)
     : User(email), is_supervised_(false) {
   set_can_lock(true);
   set_display_email(email);
 }
 
 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());
 }
 
 GuestUser::~GuestUser() {
 }
 
 UserType GuestUser::GetType() const {
   return user_manager::USER_TYPE_GUEST;
 }
@@ skipping 18 matching lines @@
 }
 
 UserType SupervisedUser::GetType() const {
   return user_manager::USER_TYPE_SUPERVISED;
 }
 
 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());
 }
 
 RetailModeUser::~RetailModeUser() {
 }
 
 UserType RetailModeUser::GetType() const {
   return user_manager::USER_TYPE_RETAIL_MODE;
 }
 
 PublicAccountUser::PublicAccountUser(const std::string& email) : User(email) {
 }
 
 PublicAccountUser::~PublicAccountUser() {
 }
 
 UserType PublicAccountUser::GetType() const {
   return user_manager::USER_TYPE_PUBLIC_ACCOUNT;
 }
 
 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:
     case user_manager::USER_TYPE_PUBLIC_ACCOUNT:
     case user_manager::USER_TYPE_SUPERVISED:
     case user_manager::USER_TYPE_KIOSK_APP:
       return false;
     default:
       NOTREACHED();
   }
   return false;
 }
 
 }  // namespace user_manager