ash: SessionControllerClient observes user image change

chrome/browser/ui/ash/session_controller_client.cc

Index: chrome/browser/ui/ash/session_controller_client.cc
diff --git a/chrome/browser/ui/ash/session_controller_client.cc b/chrome/browser/ui/ash/session_controller_client.cc
index fad60d44bd2d2948fd93239636db007fd33bbb8a..cc9bf32bd6ae9d5d7d593c94f20975fd2e6e8beb 100644
--- a/chrome/browser/ui/ash/session_controller_client.cc
+++ b/chrome/browser/ui/ash/session_controller_client.cc
@@ -27,6 +27,7 @@
 #include "services/service_manager/public/cpp/connector.h"
 #include "ui/base/resource/resource_bundle.h"
 
+using session_manager::Session;
 using session_manager::SessionManager;
 using user_manager::UserManager;
 using user_manager::User;
@@ -36,30 +37,30 @@ namespace {
 
 SessionControllerClient* g_instance = nullptr;
 
-uint32_t GetSessionId(const User* user) {
-  const UserList logged_in_users = UserManager::Get()->GetLoggedInUsers();
-  // TODO(xiyuan): Update with real session id when user session tracking
-  //     code is moved from UserManager to SessionManager.
-  for (size_t i = 0; i < logged_in_users.size(); ++i) {
-    if (logged_in_users[i] == user)
-      return i + 1;
+// Returns the session id of a given user or 0 if user has no session.
+uint32_t GetSessionId(const User& user) {
+  const AccountId& account_id = user.GetAccountId();
+  for (auto& session : SessionManager::Get()->sessions()) {
+    if (session.user_account_id == account_id)
+      return session.id;
   }
 
-  NOTREACHED();
   return 0u;
 }
 
 ash::mojom::UserSessionPtr UserToUserSession(const User& user) {

[James Cook, 2017/01/11 19:27:20]

nit: Document this function (especially why/what it means to return null)

[xiyuan, 2017/01/11 20:48:38]

Done.

+  const uint32_t user_session_id = GetSessionId(user);
+  if (user_session_id == 0u)
+    return nullptr;
+
   ash::mojom::UserSessionPtr session = ash::mojom::UserSession::New();
-  session->session_id = GetSessionId(&user);
+  session->session_id = user_session_id;
   session->type = user.GetType();
   // TODO(xiyuan): Add type map for AccountId.
   session->serialized_account_id = user.GetAccountId().Serialize();
   session->display_name = base::UTF16ToUTF8(user.display_name());
   session->display_email = user.display_email();
 
-  // TODO(xiyuan): Observe user image change and update.
-  //     Tracked in http://crbug.com/670422
   // TODO(xiyuan): Support multiple scale factor.
   session->avatar = *user.GetImage().bitmap();
   if (session->avatar.isNull()) {
@@ -80,6 +81,7 @@ void DoSwitchUser(const AccountId& account_id) {
 SessionControllerClient::SessionControllerClient() : binding_(this) {
   SessionManager::Get()->AddObserver(this);
   UserManager::Get()->AddSessionStateObserver(this);
+  UserManager::Get()->AddObserver(this);
 
   ConnectToSessionControllerAndSetClient();
   SendSessionInfoIfChanged();
@@ -95,6 +97,7 @@ SessionControllerClient::~SessionControllerClient() {
   g_instance = nullptr;
 
   SessionManager::Get()->RemoveObserver(this);
+  UserManager::Get()->RemoveObserver(this);
   UserManager::Get()->RemoveSessionStateObserver(this);
 }
 
@@ -137,6 +140,11 @@ void SessionControllerClient::UserAddedToSession(const User* added_user) {
   SendUserSession(*added_user);
 }
 
+void SessionControllerClient::OnUserImageChanged(
+    const user_manager::User& user) {
+  SendUserSession(user);
+}
+
 // static
 bool SessionControllerClient::CanLockScreen() {
   return !UserManager::Get()->GetUnlockUsers().empty();
@@ -268,7 +276,13 @@ void SessionControllerClient::SendSessionInfoIfChanged() {
 }
 
 void SessionControllerClient::SendUserSession(const User& user) {
-  session_controller_->UpdateUserSession(UserToUserSession(user));
+  ash::mojom::UserSessionPtr user_session = UserToUserSession(user);
+
+  // Bail if user has no session.

[James Cook, 2017/01/11 19:27:20]

Is this for tests or does it happen in production? This might need more comment.

[xiyuan, 2017/01/11 20:48:38]

OnUserImageChanged can be called on the login screen when UserManager loads the
user meta data (e.g. policy change caused a public session user being added, or
tests that add new users on the login screen.)

This is the only code path that runs into this situation. I could add the
session check in OnUserImageChanged. But thought it might be better to do it
here just in case we run into similar situations in the future.

Added more comments.

+  if (!user_session)
+    return;
+
+  session_controller_->UpdateUserSession(std::move(user_session));
 }
 
 void SessionControllerClient::SendUserSessionOrder() {
@@ -276,8 +290,11 @@ void SessionControllerClient::SendUserSessionOrder() {
 
   const UserList logged_in_users = user_manager->GetLoggedInUsers();
   std::vector<uint32_t> user_session_ids;
-  for (auto* user : user_manager->GetLRULoggedInUsers())
-    user_session_ids.push_back(GetSessionId(user));
+  for (auto* user : user_manager->GetLRULoggedInUsers()) {
+    const uint32_t user_session_id = GetSessionId(*user);
+    DCHECK_NE(0u, user_session_id);
+    user_session_ids.push_back(user_session_id);
+  }
 
   session_controller_->SetUserSessionOrder(user_session_ids);
 }