Adding new user menu section to the SystemTrayMenu & refactoring of user access

Adding new user menu section to the SystemTrayMenu & refactoring of user access

It is still not fully finished - but it is functional and might be useful for what you are doing (and already quite big as it is). Note that the functionality is controlled by the flag.

What is missing?
- There are no unit tests yet
- The visual Drop down box to add a new user is missing.
- The error message box which shows the error if there are already 3 users signed in is missing.
- At the moment I have ~7 lines of test code in there which I will remove before I check in. (I need them since the multi login still does not work for me)

I am not sure if you want to wait until I finish the issue - or if you'd rather want to have this to be able to rather merge early then later. I leave this up to you. If you are for the latter one, you can have at least a look at my changes and if they conflict with yours.

BUG=239201
TEST=visual

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

[Mr4D (OOO till 08-26), 2013-05-17 04:08:33]

Please see my general CL comment for more information.

I am fine either way: We can either land this first - or - I can add the
remaining work first. It works already as it is and the missing parts are local
to tray_user.cc and a few unit tests which make sense (test that only 3 users
can be added and that the user tiles only show up when the multi user mode is
activated).

I leave this up to you if it makes it easier for you for the other features or
not.

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:15: #define
ADD_TEST_USER_ACCOUNTS_FOR_MULTI_USER_TEST_ENVIRONMENT 1
As mentioned in my CL comment, this define is only for test purposes and can be
removed as soon as I can add users the proper way.

[James Cook, 2013-05-17 14:03:25]

Overall seems fine, a few specific comments

https://codereview.chromium.org/14756019/diff/1/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/14756019/diff/1/ash/ash_strings.grd#newcode218
ash/ash_strings.grd:218: <message name="IDS_ASH_STATUS_TRAY_SIGN_OUT_ALL"
desc="The label used for the button in the status tray to sign out all users of
the system.">
Nice description - the translators will be happy

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:24: // A list of eMail addresses.
nit: email (or e-mail, but http://grammarist.com/style/e-mail-email/ likes
email)

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:68: virtual void GetLoggedInUsers(UserEmailList*
users) = 0;
I suggest just returning an instance of UserEmailList - it's usually just as
efficient.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/login_status.cc
File ash/system/user/login_status.cc (right):

https://codereview.chromium.org/14756019/diff/1/ash/system/user/login_status....
ash/system/user/login_status.cc:31: if (1 <
ash::Shell::GetInstance()->session_state_delegate()->
nit: chromium-style prefers "foo == 1" to "1 == foo", so I would suggest
"NumberOfLoggedInUsers() > 1"

http://www.chromium.org/developers/coding-style#Code_Formatting

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:71: const int kMaxSimultaneousUserProfiles = 3;
Can this constant be shared with the other copy?

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:120: RoundedImageView(int corner_radius, bool
active_user);
Given that this is just a view painting, it seems odd that it knows about users.
Should |active_user| be something like |paint_active| or |full_color|?

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:178: explicit UserCard(views::ButtonListener*
listener, bool active_user);
no explicit

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:196: explicit UserView(SystemTrayItem* owner,
no explicit

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:257: SkXfermode::kLuminosity_Mode);
Aha, that's how you do it!

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:565: if
(ash::Shell::GetInstance()->session_state_delegate()->
optional nit: Sometimes for functions like this with lots of calls to
ash::Shell::GetInstance() I like to cache a copy of ash::Shell* at the top and
just use a short local variable name.

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:15: #define
ADD_TEST_USER_ACCOUNTS_FOR_MULTI_USER_TEST_ENVIRONMENT 1
On 2013/05/17 04:08:33, Mr4D wrote:
> As mentioned in my CL comment, this define is only for test purposes and can
be
> removed as soon as I can add users the proper way.

That's fine.

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:24: return
chromeos::UserManager::Get()->GetLoggedInUsers().size() +
Does this mean that all canary build users will see a test user account, or just
people with the multi-profile flag set, or just test builds?

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:59: index >=
(int)chromeos::UserManager::Get()->GetLoggedInUsers().size())
(int) -> static_cast<int>()

It hurts, but casting is supposed to hurt. :-)

[Mr4D (OOO till 08-26), 2013-05-17 16:26:40]

Addressed! Please have another look!

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:24: // A list of eMail addresses.
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> nit: email (or e-mail, but http://grammarist.com/style/e-mail-email/ likes
> email)

Done.

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:68: virtual void GetLoggedInUsers(UserEmailList*
users) = 0;
As discussed I keep it the way it is.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/login_status.cc
File ash/system/user/login_status.cc (right):

https://codereview.chromium.org/14756019/diff/1/ash/system/user/login_status....
ash/system/user/login_status.cc:31: if (1 <
ash::Shell::GetInstance()->session_state_delegate()->
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> nit: chromium-style prefers "foo == 1" to "1 == foo", so I would suggest
> "NumberOfLoggedInUsers() > 1"
> 
> http://www.chromium.org/developers/coding-style#Code_Formatting
> 

Done.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:71: const int kMaxSimultaneousUserProfiles = 3;
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> Can this constant be shared with the other copy?

Done.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:120: RoundedImageView(int corner_radius, bool
active_user);
Well ... this is a bool - and since our designers are constantly changing their
requirements I was leaving the painting logic to this function. So, if our
designers decide to put a red circle around it this would do that. (|full_color|
would be very limiting and would need to change with the implementation). If you
feel strongly I can of course change.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:178: explicit UserCard(views::ButtonListener*
listener, bool active_user);
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> no explicit

Done.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:196: explicit UserView(SystemTrayItem* owner,
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> no explicit

Done.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:257: SkXfermode::kLuminosity_Mode);
Yepp.

https://codereview.chromium.org/14756019/diff/1/ash/system/user/tray_user.cc#...
ash/system/user/tray_user.cc:565: if
(ash::Shell::GetInstance()->session_state_delegate()->
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> optional nit: Sometimes for functions like this with lots of calls to
> ash::Shell::GetInstance() I like to cache a copy of ash::Shell* at the top and
> just use a short local variable name.

Done. Note: Also that this is moving into a new place since this is the place
where more work is needed.

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:24: return
chromeos::UserManager::Get()->GetLoggedInUsers().size() +
Just with the flag turned on. But again - before submitting I was planning to
set this define to 0.

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:59: index >=
(int)chromeos::UserManager::Get()->GetLoggedInUsers().size())
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> (int) -> static_cast<int>()
> 
> It hurts, but casting is supposed to hurt. :-)

Done.

[James Cook, 2013-05-17 16:44:58]

lgtm

[Nikita (slow), 2013-05-17 17:00:31]

overall looks good, mostly nits

cc: Bartosz for SessionStateDelegate.

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:24: // A list of eMail addresses.
On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> nit: email (or e-mail, but http://grammarist.com/style/e-mail-email/ likes
> email)

We've decided to use user_id instead of email where possible (especially new
code).

There're special types of accounts like public session, kiosks etc which have
user_id but they are not really emails.

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:33: // no session in progress or no active user.
>>  no session in progress

This is a bit confusing comment because IsActiveUserSessionStrated() returns
true only after user finishes whatever setup is needed to actually go into
session.

During this "logged in but session not yet active" period user is considered to
be logged in so NumberOfLoggedInUsers() should return 1 (all backend services
like sync already take into account that fact that user is logged in).

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
File chrome/browser/ui/ash/session_state_delegate.h (left):

https://codereview.chromium.org/14756019/diff/1/chrome/browser/ui/ash/session...
chrome/browser/ui/ash/session_state_delegate.h:18: virtual bool HasActiveUser()
const OVERRIDE;
nit: I would prefer to keep this method is possible for shorter calls in places
where you only need to check whether there's at least one active user but I'm
not insisting.

Internally it could be implemented as return NumberOfLoggedInUsers();

https://codereview.chromium.org/14756019/diff/9001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/14756019/diff/9001/ash/ash_strings.grd#newcod...
ash/ash_strings.grd:218: <message name="IDS_ASH_STATUS_TRAY_SIGN_OUT_ALL"
desc="The label used for the button in the status tray to sign out all users of
the system.">
This string is not used yet, right?

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
ash/session_state_delegate.h:64: virtual const std::string
GetUserEmail(MultiProfileIndex index) const = 0;
nit: GetUserId

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
ash/session_state_delegate.h:74: virtual void SwitchActiveUser(const
std::string& email) = 0;
nit: user_id

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
ash/session_state_delegate_stub.cc:58: return "";
How about returning some stub strings here that are not empty.
Like "stub-user@domain.com"

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/login_stat...
File ash/system/user/login_status.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/login_stat...
ash/system/user/login_status.cc:33: message_id =
IDS_ASH_STATUS_TRAY_SIGN_OUT_ALL;
nit: Please add {} as condition doesn't fit on one line.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:73: // The border color of the user button.
nit: Insert empty line.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:183: void ShowActive(bool active);
nit: Comment?

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:274: 0);
nit: Please move method name to this line.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:479: // have none to allow the TrayPopupContainer
to highlight the menu line.
nit: drop second "have"

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:529: // In multiprofile mode we have also to
increase the size of the card by
nit: Please insert empty line before comment.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:532: // To make the logout button symmetrical with
the user card we also make
nit: Empty line before comment.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:610: void UserView::AddUserCard(SystemTrayItem*
owner,
Seems that this method should be split into smaller parts - it is 100 lines.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:622: user_card_ = new UserCard(this,
multiprofile_index_ == 0);
nit: Add {}

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:649: multiprofile_index_),
nit: Please move method name to this line.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:758: return new views::View();
nit: Add {}

https://codereview.chromium.org/14756019/diff/9001/ash/test/test_session_stat...
File ash/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/9001/ash/test/test_session_stat...
ash/test/test_session_state_delegate.h:31: virtual const std::string
GetUserEmail(
nit: Replace email with user_id / UserId.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:256:
temp_single_logged_in_users_.clear();
Why not use the same lru_logged_in_users_?

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:1030:
SetLRUUser(active_user_);
You already move this user in UserLoggedIn().

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:1080:
SetLRUUser(active_user_);
Not needed, is handled in UserLoggedIn()

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.h:50: virtual const UserList&
GetLRULoggedInUsers() OVERRIDE;
Please add test for UserManager.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:15: #define
ADD_TEST_USER_ACCOUNTS_FOR_MULTI_USER_TEST_ENVIRONMENT 0
Why is this needed?

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:65: index = 0;
nit: Add {}

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:67: return
chromeos::UserManager::Get()->GetLRULoggedInUsers()[
nit: Makes sense to move method name to next line:

return chromeos::UserManager::Get()->
    GetLRULoggedInUsers()[index]->display_name();

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:78: return
chromeos::UserManager::Get()->GetLRULoggedInUsers()[
nit: Same here.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:84: if
(ADD_TEST_USER_ACCOUNTS_FOR_MULTI_USER_TEST_ENVIRONMENT &&
Makes sense to extract code that returns user by index to a separate method so
that these methods will become really simple like

const gfx::ImageSkia& SessionStateDelegate::GetUserImage(
    ash::MultiProfileIndex index) const {
  return GetUserByIndex(index)->image();
}

[Mr4D (OOO till 08-26), 2013-05-17 18:45:09]

Addressed.

https://codereview.chromium.org/14756019/diff/9001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/14756019/diff/9001/ash/ash_strings.grd#newcod...
ash/ash_strings.grd:218: <message name="IDS_ASH_STATUS_TRAY_SIGN_OUT_ALL"
desc="The label used for the button in the status tray to sign out all users of
the system.">
Correct. I was adding it already for the second part of this CL (see my TODO
comments in tray_user.cc)

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
ash/session_state_delegate.h:64: virtual const std::string
GetUserEmail(MultiProfileIndex index) const = 0;
Actually: No. This is the eMail address for displaying. Renaming that into
GetUserId would be odd. That said - we can of course change this.

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
ash/session_state_delegate.h:74: virtual void SwitchActiveUser(const
std::string& email) = 0;
Okay - even though that is also quite odd since it is the email address. If we
consider changing that to some "ominous number" that is fine with me, but till
then email seems the better thing to use since that is what it is. I can change
later if you want. :)

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/session_state_delegate...
ash/session_state_delegate_stub.cc:58: return "";
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> How about returning some stub strings here that are not empty.
> Like "stub-user@domain.com"

Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/login_stat...
File ash/system/user/login_status.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/login_stat...
ash/system/user/login_status.cc:33: message_id =
IDS_ASH_STATUS_TRAY_SIGN_OUT_ALL;
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: Please add {} as condition doesn't fit on one line.

Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:73: // The border color of the user button.
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: Insert empty line.

Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:183: void ShowActive(bool active);
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: Comment?

Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:274: 0);
This was never asked for before. Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:479: // have none to allow the TrayPopupContainer
to highlight the menu line.
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: drop second "have"

Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:529: // In multiprofile mode we have also to
increase the size of the card by
Never heard that before - done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:532: // To make the logout button symmetrical with
the user card we also make
Ditto

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:610: void UserView::AddUserCard(SystemTrayItem*
owner,
I was shorten it. It is still a bit big, but much better.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:622: user_card_ = new UserCard(this,
multiprofile_index_ == 0);
Well... I always thought that this was okay since there isa single line in the
if and the else case. Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:649: multiprofile_index_),
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: Please move method name to this line.

Done.

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:758: return new views::View();
Ditto. Done.

https://codereview.chromium.org/14756019/diff/9001/ash/test/test_session_stat...
File ash/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/9001/ash/test/test_session_stat...
ash/test/test_session_state_delegate.h:31: virtual const std::string
GetUserEmail(
Again - I'd rather not since this is what it is. Let's discuss and I can still
change quickly after this has landed.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:256:
temp_single_logged_in_users_.clear();
Because as I was looking at the code it appears that there are some cases where
the |lru_logged_in_users_| can be empty, but there is an active user. This
active user can then later on apparently "convert" into a real user. If that
changes it could happen that the |lru_logged_in_users_| list could get more
users then there are. As such I'd rather have a dummy list for this case. An
alternative would have been to check the length of the users list for 0 before
adding a user to the LRU_ list and clearing it before. However - since the
execution order might change that seems rather asking for problems. Therefore I
have chosen to go this way. Again. If you feel strongly I can change this.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:1030:
SetLRUUser(active_user_);
Following all calls - indeed! Done!

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:1080:
SetLRUUser(active_user_);
Ditto.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.h:50: virtual const UserList&
GetLRULoggedInUsers() OVERRIDE;
As stated in my issue header: That will come with the next CL (which will add
the one missing UI element and the unit tests for UI and this).

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:15: #define
ADD_TEST_USER_ACCOUNTS_FOR_MULTI_USER_TEST_ENVIRONMENT 0
As stated - only for testing all the different menu states. Will be removed once
the menu is done (and / or I can add users through the usual means). Next CL and
it will be gone.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:65: index = 0;
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: Add {}

Done.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:67: return
chromeos::UserManager::Get()->GetLRULoggedInUsers()[
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: Makes sense to move method name to next line:
> 
> return chromeos::UserManager::Get()->
>     GetLRULoggedInUsers()[index]->display_name();

Done.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:78: return
chromeos::UserManager::Get()->GetLRULoggedInUsers()[
On 2013/05/17 17:00:32, Nikita Kostylev wrote:
> nit: Same here.

Done.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/ui/ash/sess...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:84: if
(ADD_TEST_USER_ACCOUNTS_FOR_MULTI_USER_TEST_ENVIRONMENT &&
This is only for the lifetime of this CL. My next CL will remove this again.
Wait for it :). At the moment this code is "neutral" and will not do anything as
long as the define is 0.

[commit-bot: I haz the power, 2013-05-17 18:45:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14756019/10032

[commit-bot: I haz the power, 2013-05-17 18:55:27]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nikita (slow), 2013-05-17 19:15:46]

lgtm

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:622: user_card_ = new UserCard(this,
multiprofile_index_ == 0);
On 2013/05/17 18:45:09, Mr4D wrote:
> Well... I always thought that this was okay since there isa single line in the
> if and the else case. Done.

Seems more readable when condition is on multiple lines.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals
"In general, curly braces are not required for single-line statements, but they
are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces."

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:256:
temp_single_logged_in_users_.clear();
On 2013/05/17 18:45:09, Mr4D wrote:
> Because as I was looking at the code it appears that there are some cases
where
> the |lru_logged_in_users_| can be empty, but there is an active user. This
> active user can then later on apparently "convert" into a real user.

When does this happen?

> If that
> changes it could happen that the |lru_logged_in_users_| list could get more
> users then there are. As such I'd rather have a dummy list for this case. An
> alternative would have been to check the length of the users list for 0 before
> adding a user to the LRU_ list and clearing it before. However - since the
> execution order might change that seems rather asking for problems. Therefore
I
> have chosen to go this way. Again. If you feel strongly I can change this.

I would really like to avoid just another user list if possible.

[commit-bot: I haz the power, 2013-05-17 19:37:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14756019/8005

[commit-bot: I haz the power, 2013-05-17 20:00:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-17 20:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14756019/22001

[Mr4D (OOO till 08-26), 2013-05-17 20:04:42]

Addressed - and many thanks for your quick review!

(more with the second CL :) ).

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/14756019/diff/9001/ash/system/user/tray_user....
ash/system/user/tray_user.cc:622: user_card_ = new UserCard(this,
multiprofile_index_ == 0);
Well - so it is formulated in a way that both is okay. Either way - done.

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/14756019/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:256:
temp_single_logged_in_users_.clear();
Okay, let's address that then with the second patch.

[commit-bot: I haz the power, 2013-05-17 21:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14756019/31001

[commit-bot: I haz the power, 2013-05-17 23:50:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/14756019/44001

[commit-bot: I haz the power, 2013-05-18 09:09:43]

Change committed as 200978

[bartfab (slow), 2013-05-21 09:26:01]

https://codereview.chromium.org/14756019/diff/44001/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/44001/ash/session_state_delegat...
ash/session_state_delegate.h:64: virtual const std::string
GetUserEmail(MultiProfileIndex index) const = 0;
Ugh. I was added as a reviewer but then the CL was committed before I got a
chance to take a look at it.

Although the CL has landed, I still think there is one thing that needs
changing: We use the terms |email|, |username|, |user_id| for the same thing in
different parts of the code. We discussed this recently and settled on "user ID"
as the single term to use in the future. Could you make a follow-up CL that
modifies this code to use "UserId" instead of "Email" in its method signatures
and comments?

[Nikita (slow), 2013-05-21 09:57:36]

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:24: // A list of eMail addresses.
On 2013/05/17 17:00:31, Nikita Kostylev wrote:
> On 2013/05/17 14:03:25, James Cook (Chromium) wrote:
> > nit: email (or e-mail, but http://grammarist.com/style/e-mail-email/ likes
> > email)
> 
> We've decided to use user_id instead of email where possible (especially new
> code).
> 
> There're special types of accounts like public session, kiosks etc which have
> user_id but they are not really emails.

Some of my comments from these files seems not have been addressed.

[Mr4D (OOO till 08-26), 2013-05-21 15:48:43]

Fot the "not addressed issues": I think I have addressed all of them - except
for the eMail -> user_id change (and I have put in a comment for that):

I have two reasons why I would not be in favor of such a change:
1. email is much clearer then user_id. A user id could be a number or maybe a
GUID or what not. But I would surely not expect it to be an email address.
2. In all relevant cases this *is* an email address. For all remaining cases
(which are probably the minority of cases >10%?) this value gets - as far as I
know - ignored and makes no sense.

What about: GetUserEmail -> GetUserId and GetDisplayEmail -> GetEmail? I think
that would be much clearer and address all concerns.

As such it seems to me much clearer to name them to what they really are: email
addresses.
Ad discussed with nkostylev: If the consensus is to use user_id I will change it

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:24: // A list of eMail addresses.
Which one? I commented on every one and the email -> userid I had my doubts that
that would be a good idea and we should address that separately with a new CL
after a "global OK" to do this change.

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:24: // A list of eMail addresses.
I have two reasons why I would not be in favor of such a change:
1. email is much clearer then user_id. A user id could be a number or maybe a
GUID. But I would surely not expect it to be an email address.
2. In all relevant cases this *is* an email address. For all remaining cases
(which are probably the minority of cases >10%?) this value gets - as far as I
know - ignored and makes no sense.

As such it seems to me much clearer to name them what they are: email addresses.
That said: If the consensus is to use user_id I will change it.

https://codereview.chromium.org/14756019/diff/1/ash/session_state_delegate.h#...
ash/session_state_delegate.h:33: // no session in progress or no active user.
I looked at the code and it appeared to me that the user list can be empty, but
the function GetActiveUser (or wat ever the exact name was) would return a user.
(I can look up the exact code locations if you want). Since this function looked
only at the length of the user list it would get a 0 in that case.

https://codereview.chromium.org/14756019/diff/44001/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/14756019/diff/44001/ash/session_state_delegat...
ash/session_state_delegate.h:64: virtual const std::string
GetUserEmail(MultiProfileIndex index) const = 0;
This function is needed by the UI in order to show the logged in eMail address.
It does not show a UserID since that might change in the future to something
else then the eMail address (or the name is not clear). As such this function
should have exactly this name and nothing with UserID in it. That would be
really confusing.

I discussed this now with nkostylev and we will run this namechange (email
->userid) by the team and see for any objections. If everyone is happy I will
change.

On a side note: The UI faces the user name and the user eMail to the user. As
such using the term eMail is much clearer then user_id. Granted it only makes
sense with GAIA, but that is the main use case we care about in the UI planning
(other non logged in users won't show this).

Once everyone agrees on this name change I can change the names in a later CL.