ash: SessionControllerClient observes user image change

BUG=670422

Review-Url: https://codereview.chromium.org/2619653002
Cr-Commit-Position: refs/heads/master@{#443431}

chrome/browser/chromeos/file_manager/file_manager_browsertest.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chromeos/chromeos_switches.h"
+#include "components/session_manager/core/session_manager.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/user_manager/user_manager.h"
 
@@ -635,13 +636,13 @@ class MultiProfileFileManagerBrowserTest : public FileManagerBrowserTestBase {
 
   // Adds a new user for testing to the current session.
   void AddUser(const TestAccountInfo& info, bool log_in) {
-    user_manager::UserManager* const user_manager =
-        user_manager::UserManager::Get();
     const AccountId account_id(AccountId::FromUserEmail(info.email));
-    if (log_in)
-      user_manager->UserLoggedIn(account_id, info.hash, false);
-    user_manager->SaveUserDisplayName(account_id,
-                                      base::UTF8ToUTF16(info.display_name));
+    if (log_in) {
+      session_manager::SessionManager::Get()->CreateSession(account_id,
+                                                            info.hash);
+    }
+    user_manager::UserManager::Get()->SaveUserDisplayName(
+        account_id, base::UTF8ToUTF16(info.display_name));
     SigninManagerFactory::GetForProfile(
         chromeos::ProfileHelper::GetProfileByUserIdHash(info.hash))
         ->SetAuthenticatedAccountInfo(info.gaia_id, info.email);

chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc

@@ -27,6 +27,7 @@
 #include "chrome/test/base/testing_browser_process.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/prefs/scoped_user_pref_update.h"
+#include "components/session_manager/core/session_manager.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
@@ -40,6 +41,7 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image_skia.h"
 
+using session_manager::SessionManager;
 using wallpaper::WallpaperInfo;
 using wallpaper::WALLPAPER_LAYOUT_CENTER;
 using wallpaper::WALLPAPER_LAYOUT_CENTER_CROPPED;
@@ -114,8 +116,7 @@ class WallpaperManagerBrowserTest : public InProcessBrowserTest {
 
   // Logs in |account_id|.
   void LogIn(const AccountId& account_id, const std::string& user_id_hash) {
-    user_manager::UserManager::Get()->UserLoggedIn(account_id, user_id_hash,
-                                                   false);
+    SessionManager::Get()->CreateSession(account_id, user_id_hash);
     // Adding a secondary display creates a shelf on that display, which
     // assumes a shelf on the primary display if the user was logged in.
     ash::WmShell::Get()->CreateShelf();
@@ -125,8 +126,7 @@ class WallpaperManagerBrowserTest : public InProcessBrowserTest {
   // Logs in |account_id| and sets it as child account.
   void LogInAsChild(const AccountId& account_id,
                     const std::string& user_id_hash) {
-    user_manager::UserManager::Get()->UserLoggedIn(account_id, user_id_hash,
-                                                   false);
+    SessionManager::Get()->CreateSession(account_id, user_id_hash);
     user_manager::User* user =
         user_manager::UserManager::Get()->FindUserAndModify(account_id);
     user_manager::UserManager::Get()->ChangeUserChildStatus(
@@ -454,7 +454,7 @@ class WallpaperManagerBrowserTestCrashRestore
 
 IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTestCrashRestore,
                        PRE_RestoreWallpaper) {
-  LogIn(test_account_id1_, kTestUser1Hash);
+  // No need to explicitly login for crash-n-restore.
   wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
 }
 
@@ -473,12 +473,6 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTestCrashRestore,
 
 class WallpaperManagerBrowserTestCacheUpdate
     : public WallpaperManagerBrowserTest {
- public:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitchASCII(switches::kLoginUser,
-                                    test_account_id1_.GetUserEmail());
-    command_line->AppendSwitchASCII(switches::kLoginProfile, "user");
-  }
  protected:
   // Creates a test image of size 1x1.
   gfx::ImageSkia CreateTestImage(SkColor color) {
@@ -544,6 +538,8 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTestCacheUpdate,
 // wallpaper cache should not be deleted.
 IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTestCacheUpdate,
                        VerifyWallpaperCache) {
+  LogIn(test_account_id1_, kTestUser1Hash);
+
   WallpaperManager* wallpaper_manager = WallpaperManager::Get();
 
   // Force load initial wallpaper
@@ -766,8 +762,8 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTest, SmallGuestWallpaper) {
   if (!ash::test::AshTestHelper::SupportsMultipleDisplays())
     return;
   CreateCmdlineWallpapers();
-  user_manager::UserManager::Get()->UserLoggedIn(
-      user_manager::GuestAccountId(), user_manager::kGuestUserName, false);
+  SessionManager::Get()->CreateSession(user_manager::GuestAccountId(),
+                                       user_manager::kGuestUserName);
   UpdateDisplay("800x600");
   WallpaperManager::Get()->SetDefaultWallpaperNow(EmptyAccountId());
   wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
@@ -781,8 +777,8 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTest, LargeGuestWallpaper) {
     return;
 
   CreateCmdlineWallpapers();
-  user_manager::UserManager::Get()->UserLoggedIn(
-      user_manager::GuestAccountId(), user_manager::kGuestUserName, false);
+  SessionManager::Get()->CreateSession(user_manager::GuestAccountId(),
+                                       user_manager::kGuestUserName);
   UpdateDisplay("1600x1200");
   WallpaperManager::Get()->SetDefaultWallpaperNow(EmptyAccountId());
   wallpaper_manager_test_utils::WaitAsyncWallpaperLoadFinished();
@@ -823,8 +819,8 @@ IN_PROC_BROWSER_TEST_F(WallpaperManagerBrowserTest,
   // Start loading the default wallpaper.
   UpdateDisplay("640x480");
   CreateCmdlineWallpapers();
-  user_manager::UserManager::Get()->UserLoggedIn(user_manager::StubAccountId(),
-                                                 "test_hash", false);
+  SessionManager::Get()->CreateSession(user_manager::StubAccountId(),
+                                       "test_hash");
 
   WallpaperManager::Get()->SetDefaultWallpaperNow(EmptyAccountId());
 

chrome/browser/download/notification/download_notification_browsertest.cc

@@ -30,6 +30,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
+#include "components/session_manager/core/session_manager.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/download_item.h"
@@ -1183,13 +1184,11 @@ class MultiProfileDownloadNotificationTest
 
   // Adds a new user for testing to the current session.
   void AddUser(const TestAccountInfo& info, bool log_in) {
-    user_manager::UserManager* const user_manager =
-        user_manager::UserManager::Get();
-    if (log_in)
-      user_manager->UserLoggedIn(
-          AccountId::FromUserEmailGaiaId(info.email, info.gaia_id), info.hash,
-          false);
-    user_manager->SaveUserDisplayName(
+    if (log_in) {
+      session_manager::SessionManager::Get()->CreateSession(
+          AccountId::FromUserEmailGaiaId(info.email, info.gaia_id), info.hash);
+    }
+    user_manager::UserManager::Get()->SaveUserDisplayName(
         AccountId::FromUserEmailGaiaId(info.email, info.gaia_id),
         base::UTF8ToUTF16(info.display_name));
     SigninManagerFactory::GetForProfile(

chrome/browser/ui/ash/chrome_new_window_client_browsertest.cc

@@ -9,8 +9,8 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "components/session_manager/core/session_manager.h"
 #include "components/signin/core/account_id/account_id.h"
-#include "components/user_manager/user_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/window.h"
 
@@ -28,8 +28,8 @@ using ChromeNewWindowClientBrowserTest = InProcessBrowserTest;
 // should open a new window.
 IN_PROC_BROWSER_TEST_F(ChromeNewWindowClientBrowserTest,
                        NewWindowForActiveWindowProfileTest) {
-  user_manager::UserManager::Get()->UserLoggedIn(
-      AccountId::FromUserEmail(kTestUserName1), kTestUserName1, false);
+  session_manager::SessionManager::Get()->CreateSession(
+      AccountId::FromUserEmail(kTestUserName1), kTestUserName1);
   Profile* profile1 = ProfileManager::GetActiveUserProfile();
   Browser* browser1 = CreateBrowser(profile1);
   // The newly created window should be created for the current active profile.
@@ -39,8 +39,8 @@ IN_PROC_BROWSER_TEST_F(ChromeNewWindowClientBrowserTest,
       profile1);
 
   // Login another user and make sure the current active user changes.
-  user_manager::UserManager::Get()->UserLoggedIn(
-      AccountId::FromUserEmail(kTestUserName2), kTestUserName2, false);
+  session_manager::SessionManager::Get()->CreateSession(
+      AccountId::FromUserEmail(kTestUserName2), kTestUserName2);
   Profile* profile2 = ProfileManager::GetActiveUserProfile();
   EXPECT_NE(profile1, profile2);
 

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,29 +37,31 @@ 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;
 }
 
+// Creates a mojom::UserSession for the given user. Returns nullptr if there is
+// no user session started for the given user.
 ash::mojom::UserSessionPtr UserToUserSession(const User& user) {
+  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();
   session->account_id = user.GetAccountId();
   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()) {
@@ -79,6 +82,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();
@@ -94,6 +98,7 @@ SessionControllerClient::~SessionControllerClient() {
   g_instance = nullptr;
 
   SessionManager::Get()->RemoveObserver(this);
+  UserManager::Get()->RemoveObserver(this);
   UserManager::Get()->RemoveSessionStateObserver(this);
 }
 
@@ -128,6 +133,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();
@@ -259,7 +269,16 @@ void SessionControllerClient::SendSessionInfoIfChanged() {
 }
 
 void SessionControllerClient::SendUserSession(const User& user) {
-  session_controller_->UpdateUserSession(UserToUserSession(user));
+  ash::mojom::UserSessionPtr user_session = UserToUserSession(user);
+
+  // Bail if the user has no session. Currently the only code path that hits
+  // this condition is from OnUserImageChanged when user images are changed
+  // on the login screen (e.g. policy change that adds a public session user,
+  // or tests that create new users on the login screen).
+  if (!user_session)
+    return;
+
+  session_controller_->UpdateUserSession(std::move(user_session));
 }
 
 void SessionControllerClient::SendUserSessionOrder() {
@@ -267,8 +286,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);
 }

chrome/browser/ui/ash/session_controller_client.h

@@ -25,6 +25,7 @@ class User;
 class SessionControllerClient
     : public ash::mojom::SessionControllerClient,
       public user_manager::UserManager::UserSessionStateObserver,
+      public user_manager::UserManager::Observer,
       public session_manager::SessionManagerObserver {
  public:
   SessionControllerClient();
@@ -39,6 +40,9 @@ class SessionControllerClient
   void ActiveUserChanged(const user_manager::User* active_user) override;
   void UserAddedToSession(const user_manager::User* added_user) override;
 
+  // user_manager::UserManager::Observer
+  void OnUserImageChanged(const user_manager::User& user) override;
+
   // session_manager::SessionManagerObserver:
   void OnSessionStateChanged() override;