ash: SessionControllerClient observes user image change

chrome/browser/ui/ash/session_controller_client.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/ash/session_controller_client.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "ash/public/cpp/session_types.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/login/users/multi_profile_user_controller.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/ash_util.h"
 #include "chrome/browser/ui/ash/multi_user/user_switch_util.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/theme_resources.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "components/prefs/pref_service.h"
 #include "components/session_manager/core/session_manager.h"
 #include "content/public/common/service_manager_connection.h"
 #include "content/public/common/service_names.mojom.h"
 #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;
 using user_manager::UserList;
 
 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()) {
     session->avatar = *ResourceBundle::GetSharedInstance()
                            .GetImageSkiaNamed(IDR_PROFILE_PICTURE_LOADING)
                            ->bitmap();
   }
 
   return session;
 }
 
 void DoSwitchUser(const AccountId& account_id) {
   UserManager::Get()->SwitchActiveUser(account_id);
 }
 
 }  // namespace
 
 SessionControllerClient::SessionControllerClient() : binding_(this) {
   SessionManager::Get()->AddObserver(this);
   UserManager::Get()->AddSessionStateObserver(this);
+  UserManager::Get()->AddObserver(this);
 
   ConnectToSessionControllerAndSetClient();
   SendSessionInfoIfChanged();
   // User sessions and their order will be sent via UserSessionStateObserver
   // even for crash-n-restart.
 
   DCHECK(!g_instance);
   g_instance = this;
 }
 
 SessionControllerClient::~SessionControllerClient() {
   DCHECK_EQ(this, g_instance);
   g_instance = nullptr;
 
   SessionManager::Get()->RemoveObserver(this);
+  UserManager::Get()->RemoveObserver(this);
   UserManager::Get()->RemoveSessionStateObserver(this);
 }
 
 void SessionControllerClient::RequestLockScreen() {
   DoLockScreen();
 }
 
 void SessionControllerClient::SwitchActiveUser(
     const std::string& serialized_account_id) {
   // TODO(xiyuan): Add type map for AccountId.
@@ skipping 22 matching lines @@
   }
 
   SendUserSessionOrder();
 }
 
 void SessionControllerClient::UserAddedToSession(const User* added_user) {
   SendSessionInfoIfChanged();
   SendUserSession(*added_user);
 }
 
+void SessionControllerClient::OnUserImageChanged(
+    const user_manager::User& user) {
+  SendUserSession(user);
+}
+
 // static
 bool SessionControllerClient::CanLockScreen() {
   return !UserManager::Get()->GetUnlockUsers().empty();
 }
 
 // static
 bool SessionControllerClient::ShouldLockScreenAutomatically() {
   // TODO(xiyuan): Observe prefs::kEnableAutoScreenLock and update ash.
   // Tracked in http://crbug.com/670423
   const UserList logged_in_users = UserManager::Get()->GetLoggedInUsers();
@@ skipping 111 matching lines @@
   info->add_user_session_policy = GetAddUserSessionPolicy();
   info->state = session_manager->session_state();
 
   if (info != last_sent_session_info_) {
     last_sent_session_info_ = info->Clone();
     session_controller_->SetSessionInfo(std::move(info));
   }
 }
 
 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() {
   UserManager* const user_manager = UserManager::Get();
 
   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);
 }