user_manager component: Add UserManagerBase class.

user_manager component: Add UserManagerBase class.

UserManagerBase contains common methods extracted from UserManagerImpl.
UserManagerImpl was renamed to ChromeUserManager and inherits UserManagerBase.

Misc refactoring:
* Move HasBrowserStarted() to SessionManager
* Add generic getter SessionManager::Get() which makes it possible for concrete clients don't depend on instance ownership like g_browser_process->platform_part()->SessionManager().
* Moved CRLSet code out of UserManager::UserLoggedIn(), to UserSessionManager.

BUG=387614
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286187

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287153

[Nikita (slow), 2014-07-28 07:21:08]

Please take a look,

Dmitry - whole CL,

Colin - components/session_manager.gypi

[Nikita (slow), 2014-07-28 09:20:27]

On 2014/07/28 07:21:08, Nikita Kostylev wrote:
> Please take a look,
> 
> Dmitry - whole CL,
> 
> Colin - components/session_manager.gypi

blundell@ is out of office

+jochen@chromium.org for 

components/OWNERS
components/session_manager.gypi

[Dmitry Polukhin, 2014-07-28 09:32:39]

LGTM

Please fix some minor inconsistencies that were before but since you are
touching this code anyway please fix them in this CL and some other you already
fixed.

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

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:709:
InitializeCRLSetFetcherForPrimaryUser(user);
Optional. I would make it InitializeCRLSetFetcher and move under 'if' above that
do RLZ initialization for primary user only.

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

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager.cc:272: delegate))) {
Single if condition on 6 lines is too much. Please simplify by introduce
temporary variable for example.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/mock_user_manager.cc (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.cc:31: user_manager::User*
MockUserManager::GetLoggedInUser() {
const?

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.cc:47: user_manager::User*
MockUserManager::GetActiveUser() {
const?

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/mock_user_manager.h (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.h:86: virtual
user_manager::User* GetLoggedInUser() OVERRIDE;
Why it is no const?

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.h:88: virtual
user_manager::User* GetActiveUser() OVERRIDE;
Ditto.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/user_manager_base.cc (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/user_manager_base.cc:150: if
(lru_logged_in_users_.empty() && active_user_) {
Perhaps it should be assert that active user is not in the list.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/user_manager_base.cc:1026:
base::Bind(&UserManagerBase::DoUpdateAccountLocale,
Please simplify, two enclosed bind calls is too much.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/user_manager_base.h (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/user_manager_base.h:38: virtual const
user_manager::UserList& GetLRULoggedInUsers() OVERRIDE;
const?

https://codereview.chromium.org/417623002/diff/210001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/417623002/diff/210001/components/session_mana...
components/session_manager/core/session_manager.h:71: static SessionManager*
instance;
Please comment about lifetime and ownership.

[Dmitry Polukhin, 2014-07-28 09:32:42]

LGTM

Please fix some minor inconsistencies that were before but since you are
touching this code anyway please fix them in this CL and some other you already
fixed.

[jochen (gone - plz use gerrit), 2014-07-29 08:48:41]

components lgtm with nit

https://codereview.chromium.org/417623002/diff/230001/components/session_mana...
File components/session_manager/core/session_manager.cc (right):

https://codereview.chromium.org/417623002/diff/230001/components/session_mana...
components/session_manager/core/session_manager.cc:26:
SessionManager::SetInstance(NULL);
DCHECK() that the instance == this?

[Nikita (slow), 2014-07-29 09:27:28]

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

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:709:
InitializeCRLSetFetcherForPrimaryUser(user);
On 2014/07/28 09:32:38, Dmitry Polukhin wrote:
> Optional. I would make it InitializeCRLSetFetcher and move under 'if' above
that
> do RLZ initialization for primary user only.

Done.

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

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/chrome_user_manager.cc:272: delegate))) {
On 2014/07/28 09:32:38, Dmitry Polukhin wrote:
> Single if condition on 6 lines is too much. Please simplify by introduce
> temporary variable for example.

Done.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/mock_user_manager.cc (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.cc:31: user_manager::User*
MockUserManager::GetLoggedInUser() {
On 2014/07/28 09:32:39, Dmitry Polukhin wrote:
> const?

../../chrome/browser/chromeos/login/users/user_manager.h:188:31: error:
functions that differ only in their return type cannot be overloaded
  virtual user_manager::User* GetLoggedInUser() const = 0;

This is because there're two functions:

const user_manager::User* MockUserManager::GetLoggedInUser() const;
user_manager::User* MockUserManager::GetLoggedInUser();

I'll do a separate spot-check (not in this CL though) to understand why certain
callers need non-const return type.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.cc:47: user_manager::User*
MockUserManager::GetActiveUser() {
On 2014/07/28 09:32:38, Dmitry Polukhin wrote:
> const?

Same here.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/mock_user_manager.h (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.h:86: virtual
user_manager::User* GetLoggedInUser() OVERRIDE;
On 2014/07/28 09:32:39, Dmitry Polukhin wrote:
> Why it is no const?

Replied in another comment.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/mock_user_manager.h:88: virtual
user_manager::User* GetActiveUser() OVERRIDE;
On 2014/07/28 09:32:39, Dmitry Polukhin wrote:
> Ditto.

Replied in another comment.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/user_manager_base.cc (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/user_manager_base.cc:150: if
(lru_logged_in_users_.empty() && active_user_) {
On 2014/07/28 09:32:39, Dmitry Polukhin wrote:
> Perhaps it should be assert that active user is not in the list.

I've checked that temp_single_logged_in_users_ is not needed at all since
active_user_ is always added to lru_logged_in_users_.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/user_manager_base.cc:1026:
base::Bind(&UserManagerBase::DoUpdateAccountLocale,
On 2014/07/28 09:32:39, Dmitry Polukhin wrote:
> Please simplify, two enclosed bind calls is too much.

Done.

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/user_manager_base.h (right):

https://codereview.chromium.org/417623002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/user_manager_base.h:38: virtual const
user_manager::UserList& GetLRULoggedInUsers() OVERRIDE;
On 2014/07/28 09:32:39, Dmitry Polukhin wrote:
> const?

Done.

https://codereview.chromium.org/417623002/diff/210001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/417623002/diff/210001/components/session_mana...
components/session_manager/core/session_manager.h:71: static SessionManager*
instance;
On 2014/07/28 09:32:39, Dmitry Polukhin wrote:
> Please comment about lifetime and ownership.

Done.

https://codereview.chromium.org/417623002/diff/230001/components/session_mana...
File components/session_manager/core/session_manager.cc (right):

https://codereview.chromium.org/417623002/diff/230001/components/session_mana...
components/session_manager/core/session_manager.cc:26:
SessionManager::SetInstance(NULL);
On 2014/07/29 08:48:40, jochen wrote:
> DCHECK() that the instance == this?

Done.

[Nikita (slow), 2014-07-29 10:44:25]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-07-29 10:45:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/417623002/290001

[commit-bot: I haz the power, 2014-07-29 13:05:27]

Change committed as 286187

[Nikita (slow), 2014-08-01 13:09:30]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-01 13:11:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/417623002/450001

[commit-bot: I haz the power, 2014-08-01 19:31:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[Nikita (slow), 2014-08-01 21:36:30]

The CQ bit was unchecked by nkostylev@chromium.org

[Nikita (slow), 2014-08-01 21:36:32]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-08-01 21:38:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/417623002/450001

[commit-bot: I haz the power, 2014-08-02 05:44:17]

Change committed as 287153