UserManager stack refactoring.

UserManager stack refactoring.

Before: while UserManager and UserManagerBase were componentized to user_manager namespace, FakeUserManager was staying at chromeos namespace.

After: UserManager Chrome OS-related interface moved to a separate class ChromeUserManagerInterface. Mixing it to UserManagerBase and FakeUserManager we're getting chrome OS versions of them. That allows us to move UserManagerBase and FakeUserManager into component user_manager.

Main changes are located at .*user_manager.* files. Such as chrome_user_manager, chrome_user_manager_interface, fake_chrome_user_manager, user_manager_base etc.
Other files (mostly tests) were changed due to changes in include paths and FakeUserManager class renaming.

BUG=443119
Committed: https://crrev.com/793f3021c1d74a754af1d1080b0e412005f1413b
Cr-Commit-Position: refs/heads/master@{#314532}

[merkulova, 2014-12-22 13:15:11]

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

[merkulova, 2014-12-22 13:15:39]

Dirty draft. Build problems.

[merkulova, 2014-12-24 11:36:46]

One more build problem at FakeChromeUserManager

[ygorshenin1, 2015-01-12 10:42:14]

Ping me as soon as the CL will be ready for review.

[merkulova, 2015-01-20 15:21:10]

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

[merkulova, 2015-01-20 15:21:11]

PTAL

[Nikita (slow), 2015-01-20 16:39:28]

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:27: static
user_manager::UserList GetUsersAllowedAsSupervisedUserManagers(
As discussed makes sense to leave in ChromeUserManager and call as
ChromeUserManager::GetUsersAllowedAsSupervisedUserManagers() from fake instance.

[Nikita (slow), 2015-01-20 18:18:22]

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.h:67: virtual
MultiProfileUserController* GetMultiProfileUserController() override;
nit: you don't need virtual here and below.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:790:
user_manager_enabler_.reset(new chromeos::ScopedUserManagerEnabler(
It looks like this whole block is under

#if defined(OS_CHROMEOS) - see line 633
ends at line 933

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:83:
chromeos::ChromeUserManager::Get()
nit: drop chromeos namespace

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc:214:
chromeos::ChromeUserManager::Get()
nit: drop chromeos

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
File components/user_manager/fake_user_manager.cc (right):

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.cc:17: const base::Closure& task,
nit: please run git cl format for your CL

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.cc:35: owner_email_("") {
nit: std::string()

[Nikita (slow), 2015-01-20 18:22:13]

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
nit: Can you please try to use lower value for
git cl upload --similarity=xx

Default is 50 and lowering that should help to make fake_chrome_user_manager.*
files look like code moves and not like completely new files.

[Nikita (slow), 2015-01-20 18:22:13]

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
nit: Can you please try to use lower value for
git cl upload --similarity=xx

Default is 50 and lowering that should help to make fake_chrome_user_manager.*
files look like code moves and not like completely new files.

[ygorshenin1, 2015-01-20 18:33:41]

First bunch of comments, please, address them.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:963:
"chromeos/login/users/fake_supervised_user_manager.cc",
Sort, please.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:196:
user_manager_(new chromeos::FakeChromeUserManager),
Could you please add '()' after FakeChromeUserManager?

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.h:26: }  // namespace
user_manager
No need in "// namespace .*" in case of forward-declarations in header files.
Could you please remove it at lines #22 and #26?

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

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager.h:10: #include
"components/user_manager/user.h"
It seems that you don't need to include "components/user_manager/user.h" here.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager.h:19:
ChromeUserManager(scoped_refptr<base::TaskRunner> task_runner,
#include "base/memory/ref_counted.h"
#include "base/task_runner.h"

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.h:67: virtual
MultiProfileUserController* GetMultiProfileUserController() override;
Could you please remove virtual keywords here? They're redundant since override
is specified.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:29:
user_manager::UserList result;
Header files are usually used for declarations, not definitions, especially for
non-trivial methods.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:25: // Can't use
STLDeleteElements because of the private destructor of User.
Feel free to mark User::~User() as public. BTW, why not to use vector of
linked_ptr's instead of raw pointers?

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:72: email,
ProfileHelper::GetUserIdHashByUserIdForTesting(email), false);
Could you please add /* browser_restart */ after false?

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:86: return NULL;
Modern fancy approach is to use nullptr instead of NULL.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:102: for
(user_manager::UserList::const_iterator it = user_list_.begin();
for (User* user : user_list_)
  user->set_is_active(user->email() == active_user_id_);

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:142:
result.push_back(*it);
Could you please add curly braces around branch?

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:29: virtual
~FakeChromeUserManager();
Modern fancy approach is to write:
~FakeChromeUserManager() override;

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:32: virtual const
user_manager::User* AddUser(const std::string& email) override;
Add a comment describing from which class this method is overridden?

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:34: // Create and
add a kiosk app user.
Could you please move non-virtual methods above or below overridden?

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:47: virtual
MultiProfileUserController* GetMultiProfileUserController() override;
Could you please remove virtual keyboard for all methods with override
specifier?

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:61:
user_manager::RemoveUserDelegate* delegate) override {
Virtual methods are considered to be non-lightweight, so don't inline them. The
same for other methods.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc:47:
#include "net/base/net_errors.h"
Why do you need this?

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
File components/user_manager/fake_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.h:130: // If set this is the active
user. If empty, the first created user is the
Nit: add an empty line before comment.

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.h:132: std::string active_user_id_;
Why not to store a pointer to a user instead of string?

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.h:139: std::string owner_email_;
Why not to just return std::string() in GetOwnerEmail()?

[merkulova, 2015-01-21 14:10:34]

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:963:
"chromeos/login/users/fake_supervised_user_manager.cc",
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Sort, please.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:196:
user_manager_(new chromeos::FakeChromeUserManager),
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Could you please add '()' after FakeChromeUserManager?

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.h:26: }  // namespace
user_manager
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> No need in "// namespace .*" in case of forward-declarations in header files.
> Could you please remove it at lines #22 and #26?

Done.

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

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager.h:10: #include
"components/user_manager/user.h"
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> It seems that you don't need to include "components/user_manager/user.h" here.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager.h:19:
ChromeUserManager(scoped_refptr<base::TaskRunner> task_runner,
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> #include "base/memory/ref_counted.h"
> #include "base/task_runner.h"

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.h:67: virtual
MultiProfileUserController* GetMultiProfileUserController() override;
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Could you please remove virtual keywords here? They're redundant since
override
> is specified.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.h:67: virtual
MultiProfileUserController* GetMultiProfileUserController() override;
On 2015/01/20 18:18:22, Nikita wrote:
> nit: you don't need virtual here and below.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:27: static
user_manager::UserList GetUsersAllowedAsSupervisedUserManagers(
On 2015/01/20 16:39:28, Nikita wrote:
> As discussed makes sense to leave in ChromeUserManager and call as
> ChromeUserManager::GetUsersAllowedAsSupervisedUserManagers() from fake
instance.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:29:
user_manager::UserList result;
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Header files are usually used for declarations, not definitions, especially
for
> non-trivial methods.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:25: // Can't use
STLDeleteElements because of the private destructor of User.
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Feel free to mark User::~User() as public. BTW, why not to use vector of
> linked_ptr's instead of raw pointers?

Removed the d-tor.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:72: email,
ProfileHelper::GetUserIdHashByUserIdForTesting(email), false);
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Could you please add /* browser_restart */ after false?

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:86: return NULL;
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Modern fancy approach is to use nullptr instead of NULL.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:102: for
(user_manager::UserList::const_iterator it = user_list_.begin();
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> for (User* user : user_list_)
>   user->set_is_active(user->email() == active_user_id_);

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:142:
result.push_back(*it);
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Could you please add curly braces around branch?

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
On 2015/01/20 18:22:13, Nikita wrote:
> nit: Can you please try to use lower value for
> git cl upload --similarity=xx
> 
> Default is 50 and lowering that should help to make fake_chrome_user_manager.*
> files look like code moves and not like completely new files.

Donne. Used 40

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:29: virtual
~FakeChromeUserManager();
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Modern fancy approach is to write:
> ~FakeChromeUserManager() override;

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:32: virtual const
user_manager::User* AddUser(const std::string& email) override;
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Add a comment describing from which class this method is overridden?

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:34: // Create and
add a kiosk app user.
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Could you please move non-virtual methods above or below overridden?

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:47: virtual
MultiProfileUserController* GetMultiProfileUserController() override;
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Could you please remove virtual keyboard for all methods with override
> specifier?

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:61:
user_manager::RemoveUserDelegate* delegate) override {
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Virtual methods are considered to be non-lightweight, so don't inline them.
The
> same for other methods.

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_unittest.cc:47:
#include "net/base/net_errors.h"
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Why do you need this?

Removed.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:790:
user_manager_enabler_.reset(new chromeos::ScopedUserManagerEnabler(
On 2015/01/20 18:18:22, Nikita wrote:
> It looks like this whole block is under
> 
> #if defined(OS_CHROMEOS) - see line 633
> ends at line 933

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:83:
chromeos::ChromeUserManager::Get()
On 2015/01/20 18:18:22, Nikita wrote:
> nit: drop chromeos namespace

Done.

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/824683002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc:214:
chromeos::ChromeUserManager::Get()
On 2015/01/20 18:18:22, Nikita wrote:
> nit: drop chromeos

Done.

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
File components/user_manager/fake_user_manager.cc (right):

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.cc:17: const base::Closure& task,
On 2015/01/20 18:18:22, Nikita wrote:
> nit: please run git cl format for your CL

Done.

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.cc:35: owner_email_("") {
On 2015/01/20 18:18:22, Nikita wrote:
> nit: std::string()

Done.

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
File components/user_manager/fake_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.h:130: // If set this is the active
user. If empty, the first created user is the
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Nit: add an empty line before comment.

Done.

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.h:132: std::string active_user_id_;
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Why not to store a pointer to a user instead of string?
Left as discussed offline.

https://codereview.chromium.org/824683002/diff/180001/components/user_manager...
components/user_manager/fake_user_manager.h:139: std::string owner_email_;
On 2015/01/20 18:33:40, ygorshenin1 wrote:
> Why not to just return std::string() in GetOwnerEmail()?

Done.

[merkulova, 2015-01-21 14:13:45]

Patchset #11 (id:200001) has been deleted

[Nikita (slow), 2015-01-22 10:37:15]

lgtm

[merkulova, 2015-01-22 15:15:24]

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

[merkulova, 2015-01-22 15:20:45]

+sky@ as src/chrome owner for covering massive rename in tests.

[sky, 2015-01-22 18:46:03]

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:964: "chromeos/login/users/fake_chrome_user_manager.cc",
Maintain sort order.

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:20: class
ChromeUserManagerInterface {
Shouldn't this be named ChromeOSUserManagerInterface? Alternatively, why is
Chrome/CHromeos important here? Why not UserManagerInterface?

[ygorshenin1, 2015-01-23 10:18:03]

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.h (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.h:25: class FakeUserManager;
This name is not used in the header file, could you please remove the forward
declaration?

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:101: if
(users_.size() && !active_user_id_.empty()) {
Could you please change users_.size() by !users_.empty()?

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:43: // Create and
add a new user. user_manager::FakeUserManager overrides.
Comment about method's semantics should be in the base class. Here should be
only 'overrides' comment.

[merkulova, 2015-01-23 12:59:09]

PTAL

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:964: "chromeos/login/users/fake_chrome_user_manager.cc",
On 2015/01/22 18:46:02, sky wrote:
> Maintain sort order.

Done.

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.h (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.h:25: class FakeUserManager;
On 2015/01/23 10:18:03, ygorshenin1 wrote:
> This name is not used in the header file, could you please remove the forward
> declaration?

Done.

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:20: class
ChromeUserManagerInterface {
On 2015/01/22 18:46:02, sky wrote:
> Shouldn't this be named ChromeOSUserManagerInterface? Alternatively, why is
> Chrome/CHromeos important here? Why not UserManagerInterface?

1. Yes, it's ChromeOS related. I kept Chrome-names as it's used in all 
chromeos-related implementations.
2. It's not simply UserManager-name as with 'Chrome-' we traditionally express
the fact that this class of user manager is ChromeOS specific. And it keeps only
additional signatures for ChromeOS part.

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc:101: if
(users_.size() && !active_user_id_.empty()) {
On 2015/01/23 10:18:03, ygorshenin1 wrote:
> Could you please change users_.size() by !users_.empty()?

Done.

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/fake_chrome_user_manager.h (right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/fake_chrome_user_manager.h:43: // Create and
add a new user. user_manager::FakeUserManager overrides.
On 2015/01/23 10:18:03, ygorshenin1 wrote:
> Comment about method's semantics should be in the base class. Here should be
> only 'overrides' comment.

Done.

[ygorshenin1, 2015-01-23 14:00:06]

LGTM

[sky, 2015-01-23 17:51:44]

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:20: class
ChromeUserManagerInterface {
On 2015/01/23 12:59:09, merkulova wrote:
> On 2015/01/22 18:46:02, sky wrote:
> > Shouldn't this be named ChromeOSUserManagerInterface? Alternatively, why is
> > Chrome/CHromeos important here? Why not UserManagerInterface?
> 
> 1. Yes, it's ChromeOS related. I kept Chrome-names as it's used in all 
> chromeos-related implementations.
> 2. It's not simply UserManager-name as with 'Chrome-' we traditionally express
> the fact that this class of user manager is ChromeOS specific. And it keeps
only
> additional signatures for ChromeOS part. 


Not all chromeos specific classes have chromeos in them. Chrome/ChromeOS in the
name is useful if you have a class with the same name else where and want to
clearly distinguish the two. AFAICT that isn't the case here (this is a base
class). I don't think you need Chrome or ChromeOS in the name here and ends up
making things more verbose.

[Daniel Erat, 2015-01-23 17:57:49]

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:20: class
ChromeUserManagerInterface {
On 2015/01/23 17:51:44, sky wrote:
> On 2015/01/23 12:59:09, merkulova wrote:
> > On 2015/01/22 18:46:02, sky wrote:
> > > Shouldn't this be named ChromeOSUserManagerInterface? Alternatively, why
is
> > > Chrome/CHromeos important here? Why not UserManagerInterface?
> > 
> > 1. Yes, it's ChromeOS related. I kept Chrome-names as it's used in all 
> > chromeos-related implementations.
> > 2. It's not simply UserManager-name as with 'Chrome-' we traditionally
express
> > the fact that this class of user manager is ChromeOS specific. And it keeps
> only
> > additional signatures for ChromeOS part. 
> 
> 
> Not all chromeos specific classes have chromeos in them. Chrome/ChromeOS in
the
> name is useful if you have a class with the same name else where and want to
> clearly distinguish the two. AFAICT that isn't the case here (this is a base
> class). I don't think you need Chrome or ChromeOS in the name here and ends up
> making things more verbose.

(i agree with scott here. this is already in a chromeos directory and in the
chromeos namespace, so putting it in the class name as well neems unnecessary.)

[merkulova, 2015-01-26 13:59:43]

Renamed the interface class. PTAL

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/chrome_user_manager_interface.h
(right):

https://codereview.chromium.org/824683002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager_interface.h:20: class
ChromeUserManagerInterface {
On 2015/01/23 17:51:44, sky wrote:
> On 2015/01/23 12:59:09, merkulova wrote:
> > On 2015/01/22 18:46:02, sky wrote:
> > > Shouldn't this be named ChromeOSUserManagerInterface? Alternatively, why
is
> > > Chrome/CHromeos important here? Why not UserManagerInterface?
> > 
> > 1. Yes, it's ChromeOS related. I kept Chrome-names as it's used in all 
> > chromeos-related implementations.
> > 2. It's not simply UserManager-name as with 'Chrome-' we traditionally
express
> > the fact that this class of user manager is ChromeOS specific. And it keeps
> only
> > additional signatures for ChromeOS part. 
> 
> 
> Not all chromeos specific classes have chromeos in them. Chrome/ChromeOS in
the
> name is useful if you have a class with the same name else where and want to
> clearly distinguish the two. AFAICT that isn't the case here (this is a base
> class). I don't think you need Chrome or ChromeOS in the name here and ends up
> making things more verbose.

Done.

[sky, 2015-01-26 16:55:31]

I looked at chrome/browser (not the chromeos bits) LGTM

[merkulova, 2015-01-26 17:06:55]

The CQ bit was checked by merkulova@chromium.org

[merkulova, 2015-01-26 17:06:55]

The CQ bit was unchecked by merkulova@chromium.org

[merkulova, 2015-01-26 17:07:07]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-01-26 17:08:04]

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

[commit-bot: I haz the power, 2015-01-26 17:12:07]

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

[commit-bot: I haz the power, 2015-01-26 17:12:09]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[merkulova, 2015-01-27 11:34:43]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-01-27 11:35:50]

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

[commit-bot: I haz the power, 2015-01-27 11:42:11]

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

[commit-bot: I haz the power, 2015-01-27 11:42:13]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[merkulova, 2015-01-28 13:15:06]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-01-28 13:15:43]

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

[commit-bot: I haz the power, 2015-01-28 13:21:32]

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

[commit-bot: I haz the power, 2015-01-28 13:21:34]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[merkulova, 2015-01-30 15:47:37]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-01-30 15:48:19]

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

[commit-bot: I haz the power, 2015-01-30 16:17:54]

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

[commit-bot: I haz the power, 2015-01-30 16:17:56]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[merkulova, 2015-02-02 16:19:43]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-02 16:20:08]

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

[commit-bot: I haz the power, 2015-02-02 16:43:55]

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

[commit-bot: I haz the power, 2015-02-02 16:43:57]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[merkulova, 2015-02-03 14:55:57]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-03 14:56:25]

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

[commit-bot: I haz the power, 2015-02-03 15:00:11]

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

[commit-bot: I haz the power, 2015-02-03 15:00:13]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[merkulova, 2015-02-03 15:05:04]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-03 15:05:40]

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

[commit-bot: I haz the power, 2015-02-03 15:20:19]

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

[commit-bot: I haz the power, 2015-02-03 15:20:21]

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

[merkulova, 2015-02-03 15:26:04]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-03 15:26:45]

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

[commit-bot: I haz the power, 2015-02-03 15:46:19]

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

[commit-bot: I haz the power, 2015-02-03 15:46:21]

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

[merkulova, 2015-02-04 08:58:03]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2015-02-04 08:58:26]

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

[commit-bot: I haz the power, 2015-02-04 10:19:07]

Committed patchset #23 (id:460001)

[commit-bot: I haz the power, 2015-02-04 10:20:03]

Patchset 23 (id:??) landed as
https://crrev.com/793f3021c1d74a754af1d1080b0e412005f1413b
Cr-Commit-Position: refs/heads/master@{#314532}