Implemented system tray UI for new account management.

Implemented system tray UI for new account management.

* Added new mode in TrayUser for the case when new account management is
  enabled (--new-profile-management flag). In fact TrayUser is now supporting
  four different modes, depending of states of |multi-profiles| flag and
  |new-profile-management| flag.
* Massive refactoring were made in tray_user.cc to isolate UserCardView
  creation in separate class and make code more clear.
* UI for the cases when new account management is disabled remained without
  changes.

Known issues:
* There are no tests for new UI. Hopefully old UI is covered by tests already.
* New UI is not accessible yet.
* Stub implementation of UserAccountsDelegate is used for backend.

BUG=344844
TEST=manually

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

[dzhioev (left Google), 2014-03-25 13:42:40]

Hi, please review.
Here are mocks: https://folio.googleplex.com/chrome-ux/mocks/201-cros-mirror
and video with current implementation:
https://drive.google.com/a/google.com/file/d/0B7oEo_ZXZsklSDZ2UGFxXzRHeW8/edi...
Thanks.

[Mr4D (OOO till 08-26), 2014-03-25 14:11:48]

Is this supposed to go into M35? If so - is there any way to put the new feature
behind a flag?

[Mr4D (OOO till 08-26), 2014-03-25 14:12:35]

Never mind the flag. I should have read your comment. But is this supposed to be
in M35?

[Mr4D (OOO till 08-26), 2014-03-25 23:54:12]

Will look more tonight, but send this already now.

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:626: <message name="IDS_ASH_STATUS_TRAY_ACCOUNTS_TITLE"
desc="The label used in the footer of the menu used to manage accounts for
profile.">
What about: The footer label of the menu which is used to manager the profile
accounts.

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:629: <message name="IDS_ASH_STATUS_TRAY_ACCOUNT_LIST_TITLE"
desc="The label used as title for account list in accounts menu.">
What about: The title label for the menu which holds the accounts list.

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:632: <message
name="IDS_ASH_STATUS_TRAY_PRIMARY_ACCOUNT_LABEL" desc="The label used to display
primary user account in accounts menu.">
What about: The primary user account label in the accounts menu.

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:635: <message name="IDS_ASH_STATUS_TRAY_ADD_ACCOUNT_LABEL"
desc="The label displayed on button that launches dialog for adding new
account.">
What about: The label of the button which launches the dialog to add a new
account.

https://codereview.chromium.org/210903003/diff/20001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/210903003/diff/20001/ash/session_state_delega...
ash/session_state_delegate.h:97: // Gets the given name of the user with
|index|. Could return empty string
What about: An empty string can be returned if the user is unknown.

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:74: if (alignment != gfx::ALIGN_CENTER)
{
Looking at this I think you imply which alignments are allowed. As such you
should document that in the header definition.

[dzhioev (left Google), 2014-03-26 13:29:21]

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:626: <message name="IDS_ASH_STATUS_TRAY_ACCOUNTS_TITLE"
desc="The label used in the footer of the menu used to manage accounts for
profile.">
On 2014/03/25 23:54:13, Mr4D wrote:
> What about: The footer label of the menu which is used to manager the profile
> accounts.

Done.

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:629: <message name="IDS_ASH_STATUS_TRAY_ACCOUNT_LIST_TITLE"
desc="The label used as title for account list in accounts menu.">
On 2014/03/25 23:54:13, Mr4D wrote:
> What about: The title label for the menu which holds the accounts list.

Done.

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:632: <message
name="IDS_ASH_STATUS_TRAY_PRIMARY_ACCOUNT_LABEL" desc="The label used to display
primary user account in accounts menu.">
On 2014/03/25 23:54:13, Mr4D wrote:
> What about: The primary user account label in the accounts menu.

Done.

https://codereview.chromium.org/210903003/diff/20001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:635: <message name="IDS_ASH_STATUS_TRAY_ADD_ACCOUNT_LABEL"
desc="The label displayed on button that launches dialog for adding new
account.">
On 2014/03/25 23:54:13, Mr4D wrote:
> What about: The label of the button which launches the dialog to add a new
> account.

Done.

https://codereview.chromium.org/210903003/diff/20001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/210903003/diff/20001/ash/session_state_delega...
ash/session_state_delegate.h:97: // Gets the given name of the user with
|index|. Could return empty string
On 2014/03/25 23:54:13, Mr4D wrote:
> What about: An empty string can be returned if the user is unknown.
It's rather: "if the given name of the user is unknown."

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:74: if (alignment != gfx::ALIGN_CENTER)
{
On 2014/03/25 23:54:13, Mr4D wrote:
> Looking at this I think you imply which alignments are allowed. As such you
> should document that in the header definition.
Actually any alignment is allowed now. There was problems with ALIGN_CENTER
before my changes were made. Margin were asymmetric, and if you set alignment of
text_label_ to ALIGN_CENTER it was shifted to the right. I made margin symmetric
for ALIGN_CENTER case.

[Mr4D (OOO till 08-26), 2014-03-26 16:49:53]

I would strongly suggest to not land this before we have branched. The existing
code paths changed and spacing / layout functions have changed for all cases. 

Besides - this isn't finished and workable at this time anyways, and as such I
do not see the hurry to get it in before.

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:74: if (alignment != gfx::ALIGN_CENTER)
{
In that case: Shouldn't the two cases below then do different things for
ALIGN_LEFT / ALIGN_RIGHT?

https://codereview.chromium.org/210903003/diff/20001/ash/system/user/tray_use...
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/210903003/diff/20001/ash/system/user/tray_use...
ash/system/user/tray_user.cc:376: bool enabled_;
Seems not to be needed. Please remove.

https://codereview.chromium.org/210903003/diff/20001/ash/system/user/tray_use...
ash/system/user/tray_user.cc:395: void AddUserContent(user::LoginStatus
login_status, int multiprofile_index);
It would be great to have here a comment what it does as well.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:151: bool IsMultiProfileSupported() {
Please rename to something like IsMultiProfileSupportedAndSessionNotLocked()

Otherwise the code gets pretty hard to read (since there are already too many
functions of this kind).

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:233: class WrappingButtonView : public
views::CustomButton {
Could you change the name to "ButtonFromView"?

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:375: // Whether button enabled.
Couldn't you get this information from the button itself?

But - it appears that you don't initialize and / or use this variable. So please
remove.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:381: class UserCardView : public views::View {
Please add a comment towards what this class does.

Also - before there were already close to too many classes in this file - now
there are more (9). Could you please organize them in a way that by reading the
code you get a better idea what each one does? Like for example: in line 329 you
refer to two classes where as the first one was declared above and the second
one below. It would be great to have both classes be defined above. Maybe the
answer is to create some more cpp/h files.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:424: // This detailed view appears after click on
user card of primary user if new
maybe

... after a click on the primary users card when the new ..

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:962:
bubble_view->SetWidth(GetPreferredSize().width());
Hmm... You are changing the layout for all the different cases (guest, ..) I
would strongly suggest to postpone landing this patch till after branch point.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
File ash/system/user/user_accounts_delegate.h (right):

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:4: 
Can you please add the include guards here?

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:22: // Called when account list of user
has been changed.
.. when the account ..

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:35: // Returns user's primary account.
It has form of canonized email.
Returns the user's primary account as a canonicalized email address.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:39: // emails.
Returns a list of the user's secondary accounts in form of canonicalized email
addresses.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:43: // be not cananized, e.g. account
"test-user@gmail.com" could have display
.. but could not be canonicalized, ..

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:51: // Launches dialog that offers user
to add the new account.
Launches a dialog which lets the user add a new account.

https://codereview.chromium.org/210903003/diff/120001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/stub_user_accounts_delegate.h (right):

https://codereview.chromium.org/210903003/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/stub_user_accounts_delegate.h:25: void AddAccount(const
std::string& account);
First the class own function - then the overrides.

[dzhioev (left Google), 2014-03-26 23:48:51]

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/210903003/diff/20001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:74: if (alignment != gfx::ALIGN_CENTER)
{
On 2014/03/26 16:49:53, Mr4D wrote:
> In that case: Shouldn't the two cases below then do different things for
> ALIGN_LEFT / ALIGN_RIGHT?
For ALIGN_LEFT we add kTrayPopupDetailsLabelExtraLeftMargin to the left margin.
I don't know which behavior user will expect if he pass ALIGN_RIGHT here though
(nobody is using ALIGN_RIGHT now, and probably never will). I think we should
add extra padding to the left even for ALIGN_RIGHT.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:151: bool IsMultiProfileSupported() {
On 2014/03/26 16:49:54, Mr4D wrote:
> Please rename to something like IsMultiProfileSupportedAndSessionNotLocked()
> 
> Otherwise the code gets pretty hard to read (since there are already too many
> functions of this kind).
Renamed to IsMultiProfileSupportedAndUserActive.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:233: class WrappingButtonView : public
views::CustomButton {
On 2014/03/26 16:49:54, Mr4D wrote:
> Could you change the name to "ButtonFromView"?
Done.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:375: // Whether button enabled.
On 2014/03/26 16:49:54, Mr4D wrote:
> Couldn't you get this information from the button itself?
> 
> But - it appears that you don't initialize and / or use this variable. So
please
> remove.

Done.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:381: class UserCardView : public views::View {
Added comment.

I will reorganize classes tomorrow.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:424: // This detailed view appears after click on
user card of primary user if new
On 2014/03/26 16:49:54, Mr4D wrote:
> maybe
> 
> ... after a click on the primary users card when the new ..

Done.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:962:
bubble_view->SetWidth(GetPreferredSize().width());
On 2014/03/26 16:49:54, Mr4D wrote:
> Hmm... You are changing the layout for all the different cases (guest, ..) I
> would strongly suggest to postpone landing this patch till after branch point.
Actually I'm not. For default mode (with multi-profiles disabled and
new-account-management disabled) nothing will change, because I've only
rearranged the code and decreased dependencies between UserView and
UserCardView. As you can see UserCardView doesn't depend on UserView anymore.

I've specially compared old and new implementations. They are pixel-by-pixel
identical (http://goo.gl/lygDpr). 

The line you are pointing at is equivalent to line 606 in old file.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
File ash/system/user/user_accounts_delegate.h (right):

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:4: 
On 2014/03/26 16:49:54, Mr4D wrote:
> Can you please add the include guards here?

Done.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:22: // Called when account list of user
has been changed.
On 2014/03/26 16:49:54, Mr4D wrote:
> .. when the account ..

Done.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:35: // Returns user's primary account.
It has form of canonized email.
On 2014/03/26 16:49:54, Mr4D wrote:
> Returns the user's primary account as a canonicalized email address.

Done.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:39: // emails.
On 2014/03/26 16:49:54, Mr4D wrote:
> Returns a list of the user's secondary accounts in form of canonicalized email
> addresses.

Done.

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:43: // be not cananized, e.g. account
"test-user@gmail.com" could have display
On 2014/03/26 16:49:54, Mr4D wrote:
> .. but could not be canonicalized, ..
I think "but could not be canonicalized" is not what I meant. Changed to "but
could be uncanonical".

https://codereview.chromium.org/210903003/diff/120001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:51: // Launches dialog that offers user
to add the new account.
On 2014/03/26 16:49:54, Mr4D wrote:
> Launches a dialog which lets the user add a new account.

Done.

https://codereview.chromium.org/210903003/diff/120001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/stub_user_accounts_delegate.h (right):

https://codereview.chromium.org/210903003/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/stub_user_accounts_delegate.h:25: void AddAccount(const
std::string& account);
On 2014/03/26 16:49:54, Mr4D wrote:
> First the class own function - then the overrides.

Done.

[dzhioev (left Google), 2014-03-26 23:50:06]

On 2014/03/26 16:49:53, Mr4D wrote:
> I would strongly suggest to not land this before we have branched. The
existing
> code paths changed and spacing / layout functions have changed for all cases. 
>
Thanks. I will discuss this option tomorrow with PMs.

[Mr4D (OOO till 08-26), 2014-03-30 23:38:25]

lgtm

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:168: bool IsUserSessionBlocked() {
Please add a comment here as well (e.g. Returns true if session is blocked by
e.g. the log in screen.)

[oshima, 2014-03-31 23:07:52]

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:75: // Returns true if new account management is enabled.
Can you describe what "new" account management is?

I believe this is for multi-login, correct?

If so, it'd be better to be consistent with the method for multi-profile .

If this feature and multi-profile will never be enabled at the same
time, please consider having a enum like AccountMode { SINGLE, MULTI_PROFILE,
MULTI_LOGIN }.
This is better than having multiple boolean predicates where you have to pay
attention to impossible
state (both are true).

https://codereview.chromium.org/210903003/diff/150001/ash/system/tray/system_...
File ash/system/tray/system_tray_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/system/tray/system_...
ash/system/tray/system_tray_delegate.h:116: }  // namespace tray
namespace comment for forward decls is optional (this is ok too. just wanted to
let you know)

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:168: bool IsUserSessionBlocked() {
Also move this to the 1st place and let other utility function call this.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:394: int multiprofile_index);
virtual dtor

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:416: bool placeholder)
this variable name doesn't match the variable name passed in the caller
(for_detailed_view_). Can you document what placeholder means?

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:419: }
virtual dtor

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
File ash/system/user/user_accounts_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:39: virtual std::string
GetPrimaryAccount() = 0;
Q: Is std::string what the multi-login uses as an account id in chrome?

We have too many ways to identify a user and I wonder if we can just use
content::BrowsrContext instead.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:48: virtual std::string
GetAccountDisplayName(const std::string& account_id) = 0;
shouldn't this be string16?

https://codereview.chromium.org/210903003/diff/150001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/210903003/diff/150001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:125:
->GetLRULoggedInUsers()[index]
doesn't this line fit the previous line?

[oshima, 2014-04-01 00:30:43]

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:75: // Returns true if new account management is enabled.
On 2014/03/31 23:07:52, oshima wrote:
> Can you describe what "new" account management is?
> 
> I believe this is for multi-login, correct?
> 
> If so, it'd be better to be consistent with the method for multi-profile .
> 



> If this feature and multi-profile will never be enabled at the same
> time, please consider having a enum like AccountMode { SINGLE, MULTI_PROFILE,
> MULTI_LOGIN }.
> This is better than having multiple boolean predicates where you have to pay
> attention to impossible
> state (both are true).

nvm this part. I just chatted with mlchan and she confirmed that both can be
enabled at the same time.

[dzhioev (left Google), 2014-04-01 17:25:01]

Please review. I've split huge file tray_user.cc into several smaller files.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:168: bool IsUserSessionBlocked() {
On 2014/03/30 23:38:26, Mr4D wrote:
> Please add a comment here as well (e.g. Returns true if session is blocked by
> e.g. the log in screen.) 

Done.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:168: bool IsUserSessionBlocked() {
On 2014/03/31 23:07:52, oshima wrote:
> Also move this to the 1st place and let other utility function call this.

Done.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:394: int multiprofile_index);
On 2014/03/31 23:07:52, oshima wrote:
> virtual dtor

Done.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:416: bool placeholder)
On 2014/03/31 23:07:52, oshima wrote:
> this variable name doesn't match the variable name passed in the caller
> (for_detailed_view_). Can you document what placeholder means?

Done.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/tray_us...
ash/system/user/tray_user.cc:419: }
On 2014/03/31 23:07:52, oshima wrote:
> virtual dtor

Done.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
File ash/system/user/user_accounts_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:39: virtual std::string
GetPrimaryAccount() = 0;
On 2014/03/31 23:07:52, oshima wrote:
> Q: Is std::string what the multi-login uses as an account id in chrome?
> 
Yes, TokenService uses std::string as account ID. See
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...

> We have too many ways to identify a user and I wonder if we can just use
> content::BrowsrContext instead.
Isn't BrowserContext equivalent to Profile? If so, that will not gonna work,
because we have one-to-one correspondence between users and their profiles.
Account is not the same thing as user. One user can have several accounts.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:48: virtual std::string
GetAccountDisplayName(const std::string& account_id) = 0;
On 2014/03/31 23:07:52, oshima wrote:
> shouldn't this be string16?
Probably not. For primary account it will be "display email" which is latin. For
secondary accounts DisplayName will be equal to account_id.

https://codereview.chromium.org/210903003/diff/150001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/210903003/diff/150001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:125:
->GetLRULoggedInUsers()[index]
On 2014/03/31 23:07:52, oshima wrote:
> doesn't this line fit the previous line?
That is how 'git cl format' works. If we have chain of calls which is too long
for one line, it places every call on separate line.

[oshima, 2014-04-02 00:14:13]

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:75: // Returns true if new account management is enabled.
On 2014/03/31 23:07:52, oshima wrote:
> Can you describe what "new" account management is?
> 
> I believe this is for multi-login, correct?
> 
> If so, it'd be better to be consistent with the method for multi-profile .

Sorry if it wasn't clear but this question is still valid. I still
think "AccountManagement" is too generic.

I just chatted with Min at el, and we agreed that MultiAccount is a better name
in the code.


> If this feature and multi-profile will never be enabled at the same
> time, please consider having a enum like AccountMode { SINGLE, MULTI_PROFILE,
> MULTI_LOGIN }.
> This is better than having multiple boolean predicates where you have to pay
> attention to impossible
> state (both are true).

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
File ash/system/user/user_accounts_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:22: Observer() {}
optional: I think you can omit ctor in this case. it's up to you.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:39: virtual std::string
GetPrimaryAccount() = 0;
On 2014/04/01 17:25:02, dzhioev wrote:
> On 2014/03/31 23:07:52, oshima wrote:
> > Q: Is std::string what the multi-login uses as an account id in chrome?
> > 
> Yes, TokenService uses std::string as account ID. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...
> 
> > We have too many ways to identify a user and I wonder if we can just use
> > content::BrowsrContext instead.
> Isn't BrowserContext equivalent to Profile? If so, that will not gonna work,
> because we have one-to-one correspondence between users and their profiles.
> Account is not the same thing as user. One user can have several accounts.

That's only true in browser process and they do not have to be equivalent,
(like appshell uses BrowserContext as is).
In any case, if this is what the underlying system is using, I'm fine.

https://codereview.chromium.org/210903003/diff/150001/ash/system/user/user_ac...
ash/system/user/user_accounts_delegate.h:48: virtual std::string
GetAccountDisplayName(const std::string& account_id) = 0;
On 2014/04/01 17:25:02, dzhioev wrote:
> On 2014/03/31 23:07:52, oshima wrote:
> > shouldn't this be string16?
> Probably not. For primary account it will be "display email" which is latin.
For
> secondary accounts DisplayName will be equal to account_id.

I see. It's unfortunate that we're using different representation from
multi-profile,
but may be this is to make the difference more obvious.

https://codereview.chromium.org/210903003/diff/150001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/210903003/diff/150001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:125:
->GetLRULoggedInUsers()[index]
On 2014/04/01 17:25:02, dzhioev wrote:
> On 2014/03/31 23:07:52, oshima wrote:
> > doesn't this line fit the previous line?
> That is how 'git cl format' works. If we have chain of calls which is too long
> for one line, it places every call on separate line.

ok ok as it is.

[dzhioev (left Google), 2014-04-09 16:31:38]

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/210903003/diff/150001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:75: // Returns true if new account management is enabled.
On 2014/04/02 00:14:14, oshima wrote:
> On 2014/03/31 23:07:52, oshima wrote:
> > Can you describe what "new" account management is?
> > 
> > I believe this is for multi-login, correct?
> > 
> > If so, it'd be better to be consistent with the method for multi-profile .
> 
> Sorry if it wasn't clear but this question is still valid. I still
> think "AccountManagement" is too generic.
> 
> I just chatted with Min at el, and we agreed that MultiAccount is a better
name
> in the code.
> 
> 
> > If this feature and multi-profile will never be enabled at the same
> > time, please consider having a enum like AccountMode { SINGLE,
MULTI_PROFILE,
> > MULTI_LOGIN }.
> > This is better than having multiple boolean predicates where you have to pay
> > attention to impossible
> > state (both are true).
> 

Done. Renamed to MultiAccount.

[oshima, 2014-04-09 17:22:41]

thanks! lgtm

[dzhioev (left Google), 2014-04-09 17:42:19]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-04-09 17:42:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/210903003/230001

[commit-bot: I haz the power, 2014-04-09 17:42:43]

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

[commit-bot: I haz the power, 2014-04-09 17:42:44]

Failed to apply patch for ash/system/audio/volume_view.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/system/audio/volume_view.h
  Hunk #1 FAILED at 25.
  1 out of 1 hunk FAILED -- saving rejects to file
ash/system/audio/volume_view.h.rej

Patch:       ash/system/audio/volume_view.h
Index: ash/system/audio/volume_view.h
diff --git a/ash/system/audio/volume_view.h b/ash/system/audio/volume_view.h
index
a3e82913bf34499ece71057d81c0a137335e9d97..453ffa01977e493780b7e025510b28a395fe5375
100644
--- a/ash/system/audio/volume_view.h
+++ b/ash/system/audio/volume_view.h
@@ -25,8 +25,6 @@ class TrayAudioDelegate;
 
 namespace internal {
 
-class HoverHighlightView;
-
 namespace tray {
 
 class BarSeparator;

[dzhioev (left Google), 2014-04-10 07:07:05]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-04-10 07:07:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/210903003/240001

[commit-bot: I haz the power, 2014-04-10 13:17:20]

Change committed as 262959