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

ash/session_state_delegate.h

Index: ash/session_state_delegate.h
diff --git a/ash/session_state_delegate.h b/ash/session_state_delegate.h
index f5ee359667597ff975afbecbf3742e90d256a845..5ddad460d31e6e932d9033e50e5d8494a4a4ab48 100644
--- a/ash/session_state_delegate.h
+++ b/ash/session_state_delegate.h
@@ -5,17 +5,33 @@
 #ifndef ASH_SESSION_STATE_DELEGATE_H_
 #define ASH_SESSION_STATE_DELEGATE_H_
 
+#include <string>
+#include <vector>
+
 #include "ash/ash_export.h"
+#include "base/string16.h"
+
+namespace gfx {
+class ImageSkia;
+}  // namespace gfx
 
 namespace ash {
 
+// The index for the multi-profile item to use. The list is always LRU sorted
+// So that the index #0 is the currently active user.
+typedef int MultiProfileIndex;
+
+// A list of eMail addresses.

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

nit: email (or e-mail, but http://grammarist.com/style/e-mail-email/ likes
email)

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

Done.

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

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.

[Nikita (slow), 2013/05/21 09:57:37]

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

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

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.

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

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.

+typedef std::vector<std::string> UserEmailList;
+
 // Delegate for checking and modifying the session state.
 class ASH_EXPORT SessionStateDelegate {
  public:
   virtual ~SessionStateDelegate() {};
 
-  // Returns |true| if a session is in progress and there is an active user.
-  virtual bool HasActiveUser() const = 0;
+  // Returns the number of signed in users. If 0 is returned, there is either
+  // no session in progress or no active user.

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

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).

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

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. 

+  virtual int NumberOfLoggedInUsers() const = 0;
 
   // Returns |true| if the session has been fully started for the active user.
   // When a user becomes active, the profile and browser UI are not immediately
@@ -34,6 +50,25 @@ class ASH_EXPORT SessionStateDelegate {
 
   // Unlocks the screen.
   virtual void UnlockScreen() = 0;
+
+  // Gets the displayed name for the user with the given |index|.
+  // Note that |index| can at maximum be |NumberOfLoggedInUsers() - 1|.
+  virtual const base::string16 GetUserDisplayName(
+      MultiProfileIndex index) const = 0;
+
+  // Gets the email address for the user with the given |index|.
+  // Note that |index| can at maximum be |NumberOfLoggedInUsers() - 1|.
+  virtual const std::string GetUserEmail(MultiProfileIndex index) const = 0;
+
+  // Gets the avatar image for the user with the given |index|.
+  // Note that |index| can at maximum be |NumberOfLoggedInUsers() - 1|.
+  virtual const gfx::ImageSkia& GetUserImage(MultiProfileIndex index) const = 0;
+
+  // Returns a list of all logged in users.
+  virtual void GetLoggedInUsers(UserEmailList* users) = 0;

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

I suggest just returning an instance of UserEmailList - it's usually just as
efficient.

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

As discussed I keep it the way it is.

+
+  // Switches to another active user (if that user has already signed in).
+  virtual void SwitchActiveUser(const std::string& email) = 0;
 };
 
 }  // namespace ash