ash: Fix system tray avatar not updated regression

This is caused by racing between mojo and non-mojo code path.
The avater image is updated via SessionController mojo interface
but UserObserver interface is still a direct call and could reach
ash before the mojo message. The CL fixes the issue by replacing
UserObserver interface with SessionController observer.

BUG=704862

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

ash/BUILD.gn

@@ -428,7 +428,6 @@ component("ash") {
     "common/system/user/tray_user.h",
     "common/system/user/user_card_view.cc",
     "common/system/user/user_card_view.h",
-    "common/system/user/user_observer.h",
     "common/system/user/user_view.cc",
     "common/system/user/user_view.h",
     "common/system/web_notification/ash_popup_alignment_delegate.cc",

ash/common/session/session_controller.cc

@@ -174,8 +174,8 @@ void SessionController::UpdateUserSession(mojom::UserSessionPtr user_session) {
   }
 
   *it = std::move(user_session);
-  // TODO(xiyuan): Notify observers about meta change to replace things such as
-  //     NOTIFICATION_LOGIN_USER_IMAGE_CHANGED. http://crbug.com/670422
+  for (auto& observer : observers_)
+    observer.UserSessionUpdated((*it)->account_id);
 }
 
 void SessionController::SetUserSessionOrder(

ash/common/session/session_state_observer.h

@@ -21,6 +21,9 @@ class ASH_EXPORT SessionStateObserver {
   // Called when another user gets added to the existing session.
   virtual void UserAddedToSession(const AccountId& account_id) {}
 
+  // Called when a user session is updated, such as avatar change.
+  virtual void UserSessionUpdated(const AccountId& account_id) {}
+
   // Called when session state is changed.
   virtual void SessionStateChanged(session_manager::SessionState state) {}
 

ash/common/system/tray/system_tray_notifier.cc

@@ -18,7 +18,6 @@
 #include "ash/common/system/chromeos/virtual_keyboard/virtual_keyboard_observer.h"
 #include "ash/common/system/date/clock_observer.h"
 #include "ash/common/system/ime/ime_observer.h"
-#include "ash/common/system/user/user_observer.h"
 
 namespace ash {
 
@@ -262,24 +261,6 @@ void SystemTrayNotifier::NotifyTracingModeChanged(bool value) {
     observer.OnTracingModeChanged(value);
 }
 
-void SystemTrayNotifier::AddUserObserver(UserObserver* observer) {
-  user_observers_.AddObserver(observer);
-}
-
-void SystemTrayNotifier::RemoveUserObserver(UserObserver* observer) {
-  user_observers_.RemoveObserver(observer);
-}
-
-void SystemTrayNotifier::NotifyUserUpdate() {
-  for (auto& observer : user_observers_)
-    observer.OnUserUpdate();
-}
-
-void SystemTrayNotifier::NotifyUserAddedToSession() {
-  for (auto& observer : user_observers_)
-    observer.OnUserAddedToSession();
-}
-
 void SystemTrayNotifier::AddVirtualKeyboardObserver(
     VirtualKeyboardObserver* observer) {
   virtual_keyboard_observers_.AddObserver(observer);

ash/common/system/tray/system_tray_notifier.h

@@ -30,7 +30,6 @@ class ScreenCaptureObserver;
 class ScreenShareObserver;
 class SessionLengthLimitObserver;
 class TracingObserver;
-class UserObserver;
 class VirtualKeyboardObserver;
 
 namespace mojom {
@@ -122,12 +121,6 @@ class ASH_EXPORT SystemTrayNotifier {
   void RemoveTracingObserver(TracingObserver* observer);
   void NotifyTracingModeChanged(bool value);
 
-  // User.
-  void AddUserObserver(UserObserver* observer);
-  void RemoveUserObserver(UserObserver* observer);
-  void NotifyUserUpdate();
-  void NotifyUserAddedToSession();
-
   // Virtual keyboard.
   void AddVirtualKeyboardObserver(VirtualKeyboardObserver* observer);
   void RemoveVirtualKeyboardObserver(VirtualKeyboardObserver* observer);
@@ -149,7 +142,6 @@ class ASH_EXPORT SystemTrayNotifier {
   base::ObserverList<SessionLengthLimitObserver>
       session_length_limit_observers_;
   base::ObserverList<TracingObserver> tracing_observers_;
-  base::ObserverList<UserObserver> user_observers_;
   base::ObserverList<VirtualKeyboardObserver> virtual_keyboard_observers_;
 
   DISALLOW_COPY_AND_ASSIGN(SystemTrayNotifier);

ash/common/system/user/rounded_image_view.cc

@@ -20,14 +20,13 @@ RoundedImageView::RoundedImageView(int corner_radius) {
 
 RoundedImageView::~RoundedImageView() {}
 
-void RoundedImageView::SetImage(const gfx::ImageSkia& img,
+void RoundedImageView::SetImage(const gfx::ImageSkia& image,
                                 const gfx::Size& size) {
-  image_ = img;
   image_size_ = size;
 
   // Try to get the best image quality for the avatar.
-  resized_ = gfx::ImageSkiaOperations::CreateResizedImage(
-      image_, skia::ImageOperations::RESIZE_BEST, size);
+  resized_image_ = gfx::ImageSkiaOperations::CreateResizedImage(
+      image, skia::ImageOperations::RESIZE_BEST, size);
   if (GetWidget() && visible()) {
     PreferredSizeChanged();
     SchedulePaint();
@@ -63,8 +62,8 @@ void RoundedImageView::OnPaint(gfx::Canvas* canvas) {
   path.addRoundRect(gfx::RectToSkRect(image_bounds), kRadius);
   cc::PaintFlags flags;
   flags.setAntiAlias(true);
-  canvas->DrawImageInPath(resized_, image_bounds.x(), image_bounds.y(), path,
-                          flags);
+  canvas->DrawImageInPath(resized_image_, image_bounds.x(), image_bounds.y(),
+                          path, flags);
 }
 
 }  // namespace tray

ash/common/system/user/rounded_image_view.h

@@ -23,7 +23,7 @@ class RoundedImageView : public views::View {
 
   // Set the image that should be displayed. The image contents is copied to the
   // receiver's image.
-  void SetImage(const gfx::ImageSkia& img, const gfx::Size& size);
+  void SetImage(const gfx::ImageSkia& image, const gfx::Size& size);
 
   // Set the radii of the corners independently.
   void SetCornerRadii(int top_left,
@@ -35,9 +35,11 @@ class RoundedImageView : public views::View {
   gfx::Size GetPreferredSize() const override;
   void OnPaint(gfx::Canvas* canvas) override;
 
+  // Gets the image painted by RoundedImageView for test.
+  const gfx::ImageSkia& image_for_test() const { return resized_image_; }
+
  private:
-  gfx::ImageSkia image_;
-  gfx::ImageSkia resized_;
+  gfx::ImageSkia resized_image_;
   gfx::Size image_size_;
   int corner_radius_[4];
 

ash/common/system/user/tray_user.cc

@@ -8,7 +8,6 @@
 #include "ash/common/shelf/wm_shelf_util.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
-#include "ash/common/system/tray/system_tray_notifier.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_item_view.h"
 #include "ash/common/system/tray/tray_utils.h"
@@ -39,17 +38,10 @@ namespace ash {
 
 TrayUser::TrayUser(SystemTray* system_tray, UserIndex index)
     : SystemTrayItem(system_tray, UMA_USER),
-      user_index_(index),
-      user_(nullptr),
-      layout_view_(nullptr),
-      avatar_(nullptr),
-      label_(nullptr) {
-  Shell::Get()->system_tray_notifier()->AddUserObserver(this);
-}
+      scoped_session_observer_(this),
+      user_index_(index) {}
 
-TrayUser::~TrayUser() {
-  Shell::Get()->system_tray_notifier()->RemoveUserObserver(this);
-}
+TrayUser::~TrayUser() {}
 
 TrayUser::TestState TrayUser::GetStateForTest() const {
   if (!user_)
@@ -218,11 +210,11 @@ void TrayUser::UpdateAfterShelfAlignmentChange(ShelfAlignment alignment) {
   }
 }
 
-void TrayUser::OnUserUpdate() {
-  UpdateAvatarImage(Shell::Get()->system_tray_delegate()->GetUserLoginStatus());
+void TrayUser::ActiveUserChanged(const AccountId& account_id) {
+  UserSessionUpdated(account_id);
 }
 
-void TrayUser::OnUserAddedToSession() {
+void TrayUser::UserAddedToSession(const AccountId& account_id) {
   const SessionController* const session_controller =
       Shell::Get()->session_controller();
   // Only create views for user items which are logged in.
@@ -233,7 +225,11 @@ void TrayUser::OnUserAddedToSession() {
   UpdateLayoutOfItem();
 
   // Update the user item.
-  UpdateAvatarImage(Shell::Get()->system_tray_delegate()->GetUserLoginStatus());
+  UpdateAvatarImage(Shell::Get()->session_controller()->GetLoginStatus());
+}
+
+void TrayUser::UserSessionUpdated(const AccountId& account_id) {
+  UpdateAvatarImage(Shell::Get()->session_controller()->GetLoginStatus());
 }
 
 void TrayUser::UpdateAvatarImage(LoginStatus status) {

ash/common/system/user/tray_user.h

@@ -6,8 +6,8 @@
 #define ASH_COMMON_SYSTEM_USER_TRAY_USER_H_
 
 #include "ash/ash_export.h"
+#include "ash/common/session/session_state_observer.h"
 #include "ash/common/system/tray/system_tray_item.h"
-#include "ash/common/system/user/user_observer.h"
 #include "ash/public/cpp/session_types.h"
 #include "base/macros.h"
 
@@ -27,7 +27,7 @@ class RoundedImageView;
 class UserView;
 }
 
-class ASH_EXPORT TrayUser : public SystemTrayItem, public UserObserver {
+class ASH_EXPORT TrayUser : public SystemTrayItem, public SessionStateObserver {
  public:
   // The given |index| is the user index in a multi profile scenario. Index #0
   // is the active user, the other indices are other logged in users (if there
@@ -58,6 +58,7 @@ class ASH_EXPORT TrayUser : public SystemTrayItem, public UserObserver {
 
   // Use for access inside of tests.
   tray::UserView* user_view_for_test() const { return user_; }
+  tray::RoundedImageView* avatar_view_for_test() const { return avatar_; }
 
  private:
   // Overridden from SystemTrayItem.
@@ -68,24 +69,27 @@ class ASH_EXPORT TrayUser : public SystemTrayItem, public UserObserver {
   void UpdateAfterLoginStatusChange(LoginStatus status) override;
   void UpdateAfterShelfAlignmentChange(ShelfAlignment alignment) override;
 
-  // Overridden from UserObserver.
-  void OnUserUpdate() override;
-  void OnUserAddedToSession() override;
+  // Overridden from SessionStateObserver.
+  void ActiveUserChanged(const AccountId& account_id) override;
+  void UserAddedToSession(const AccountId& account_id) override;
+  void UserSessionUpdated(const AccountId& account_id) override;
 
   void UpdateAvatarImage(LoginStatus status);
 
   // Updates the layout of this item.
   void UpdateLayoutOfItem();
 
+  ScopedSessionStateObserver scoped_session_observer_;
+
   // The user index to use.
-  UserIndex user_index_;
+  const UserIndex user_index_;
 
-  tray::UserView* user_;
+  tray::UserView* user_ = nullptr;
 
   // View that contains label and/or avatar.
-  views::View* layout_view_;
-  tray::RoundedImageView* avatar_;
-  views::Label* label_;
+  views::View* layout_view_ = nullptr;
+  tray::RoundedImageView* avatar_ = nullptr;
+  views::Label* label_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(TrayUser);
 };

ash/common/system/user/tray_user_unittest.cc

@@ -2,12 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <utility>
 #include <vector>
 
 #include "ash/common/session/session_controller.h"
 #include "ash/common/shell_delegate.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/tray_constants.h"
+#include "ash/common/system/user/rounded_image_view.h"
 #include "ash/common/system/user/tray_user.h"
 #include "ash/common/system/user/user_view.h"
 #include "ash/common/test/test_session_controller_client.h"
@@ -18,9 +20,13 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/signin/core/account_id/account_id.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/gfx/animation/animation_container_element.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/image/image_unittest_util.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
@@ -34,6 +40,14 @@ const char* kPredefinedUserEmails[] = {
     // This is intended to be capitalized.
     "Second@tray"};
 
+// Creates an ImageSkia with a 1x rep of the given dimensions and filled with
+// the given color.
+gfx::ImageSkia CreateImageSkiaWithColor(int width, int height, SkColor color) {
+  SkBitmap bitmap = gfx::test::CreateBitmap(width, height);
+  bitmap.eraseColor(color);
+  return gfx::ImageSkia::CreateFrom1xBitmap(bitmap);
+}
+
 class TrayUserTest : public test::AshTestBase {
  public:
   TrayUserTest() = default;
@@ -267,4 +281,22 @@ TEST_F(TrayUserTest, MultiUserModeButtonClicks) {
   tray()->CloseSystemBubble();
 }
 
+// Test SessionController updates avatar image.
+TEST_F(TrayUserTest, AvatarChange) {
+  InitializeParameters(1, false);
+
+  // Expect empty avatar initially (that is how the test sets up).
+  EXPECT_TRUE(tray_user(0)->avatar_view_for_test()->image_for_test().isNull());
+
+  // Change user avatar via SessionController and verify.
+  const gfx::ImageSkia red_icon =
+      CreateImageSkiaWithColor(kTrayItemSize, kTrayItemSize, SK_ColorRED);
+  mojom::UserSessionPtr user = controller()->GetUserSession(0)->Clone();
+  user->avatar = red_icon;
+  controller()->UpdateUserSession(std::move(user));
+  EXPECT_TRUE(gfx::test::AreImagesEqual(
+      gfx::Image(red_icon),
+      gfx::Image(tray_user(0)->avatar_view_for_test()->image_for_test())));
+}
+
 }  // namespace ash

ash/common/system/user/user_observer.h

-// Copyright (c) 2012 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.
-
-#ifndef ASH_COMMON_SYSTEM_USER_USER_OBSERVER_H_
-#define ASH_COMMON_SYSTEM_USER_USER_OBSERVER_H_
-
-#include "ash/ash_export.h"
-
-namespace ash {
-
-class ASH_EXPORT UserObserver {
- public:
-  virtual ~UserObserver() {}
-
-  // A user got updated / changed.
-  virtual void OnUserUpdate() = 0;
-
-  // A user was added to the existing session.
-  virtual void OnUserAddedToSession() = 0;
-};
-
-}  // namespace ash
-
-#endif  // ASH_COMMON_SYSTEM_USER_USER_OBSERVER_H_

chrome/browser/ui/ash/system_tray_delegate_chromeos.cc

@@ -21,7 +21,6 @@
 #include "ash/common/system/ime/ime_observer.h"
 #include "ash/common/system/tray/system_tray_notifier.h"
 #include "ash/common/system/tray_accessibility.h"
-#include "ash/common/system/user/user_observer.h"
 #include "ash/shell.h"
 #include "ash/system/chromeos/rotation/tray_rotation_lock.h"
 #include "base/callback.h"
@@ -144,7 +143,6 @@ SystemTrayDelegateChromeOS::SystemTrayDelegateChromeOS()
       base::Bind(&SystemTrayDelegateChromeOS::OnAccessibilityStatusChanged,
                  base::Unretained(this)));
 
-  user_manager::UserManager::Get()->AddObserver(this);
   user_manager::UserManager::Get()->AddSessionStateObserver(this);
 }
 
@@ -207,7 +205,6 @@ SystemTrayDelegateChromeOS::~SystemTrayDelegateChromeOS() {
   if (policy_manager)
     policy_manager->core()->store()->RemoveObserver(this);
 
-  user_manager::UserManager::Get()->RemoveObserver(this);
   user_manager::UserManager::Get()->RemoveSessionStateObserver(this);
 }
 
@@ -417,7 +414,6 @@ bool SystemTrayDelegateChromeOS::GetSessionLengthLimit(
 
 void SystemTrayDelegateChromeOS::ActiveUserWasChanged() {
   SetProfile(ProfileManager::GetActiveUserProfile());
-  GetSystemTrayNotifier()->NotifyUserUpdate();
 }
 
 bool SystemTrayDelegateChromeOS::IsSearchKeyMappedToCapsLock() {
@@ -439,15 +435,6 @@ SystemTrayDelegateChromeOS::CreateRotationLockTrayItem(ash::SystemTray* tray) {
   return base::MakeUnique<ash::TrayRotationLock>(tray);
 }
 
-void SystemTrayDelegateChromeOS::UserAddedToSession(
-    const user_manager::User* active_user) {
-  GetSystemTrayNotifier()->NotifyUserAddedToSession();
-}
-
-void SystemTrayDelegateChromeOS::ActiveUserChanged(
-    const user_manager::User* /* active_user */) {
-}
-
 void SystemTrayDelegateChromeOS::UserChangedChildStatus(
     user_manager::User* user) {
   Profile* user_profile = ProfileHelper::Get()->GetProfileByUser(user);
@@ -707,13 +694,6 @@ void SystemTrayDelegateChromeOS::OnStoreError(policy::CloudPolicyStore* store) {
   UpdateEnterpriseDomain();
 }
 
-void SystemTrayDelegateChromeOS::OnUserImageChanged(
-    const user_manager::User& user) {
-  // This is also invoked on login screen when user avatar is loaded from file.
-  if (GetUserLoginStatus() != ash::LoginStatus::NOT_LOGGED_IN)
-    GetSystemTrayNotifier()->NotifyUserUpdate();
-}
-
 // Overridden from chrome::BrowserListObserver.
 void SystemTrayDelegateChromeOS::OnBrowserRemoved(Browser* browser) {
   NotifyIfLastWindowClosed();

chrome/browser/ui/ash/system_tray_delegate_chromeos.h

@@ -94,13 +94,7 @@ class SystemTrayDelegateChromeOS
   std::unique_ptr<ash::SystemTrayItem> CreateRotationLockTrayItem(
       ash::SystemTray* tray) override;
 
-  // Overridden from user_manager::UserManager::Observer:
-  void OnUserImageChanged(const user_manager::User& user) override;
-
   // Overridden from user_manager::UserManager::UserSessionStateObserver:
-  void UserAddedToSession(const user_manager::User* active_user) override;
-  void ActiveUserChanged(const user_manager::User* active_user) override;
-
   void UserChangedChildStatus(user_manager::User* user) override;
 
  private: