Fixed color management for sync and avatar icons in profile menus.

Removed the need to use the global GetInstanceForNativeUi() call
to get required colors when constructing avatars and sync icons
for profile menu views.

Also updated the profile menus such that their sync and avatar
icons are updated appropriately on theme changes.

Performed some basic cleanup, moved some functions into the base
class.

Bug: None
Change-Id: Ia3566c77653c3209265d077e64b524b7bde9c8e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2070377
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745048}

chrome/browser/ui/views/profiles/incognito_menu_view.cc

@@ -49,7 +49,6 @@ void IncognitoMenuView::BuildMenu() {
 
   SetIdentityInfo(
       ColoredImageForMenu(kIncognitoProfileIcon, icon_color),
-      /*badge=*/gfx::ImageSkia(),
       l10n_util::GetStringUTF16(IDS_INCOGNITO_PROFILE_MENU_TITLE),
       incognito_window_count > 1
           ? l10n_util::GetPluralStringFUTF16(IDS_INCOGNITO_WINDOW_COUNT_MESSAGE,

chrome/browser/ui/views/profiles/profile_menu_view.cc

@@ -140,6 +140,43 @@ void ProfileMenuView::BuildMenu() {
   BuildProfileManagementFeatureButtons();
 }
 
+gfx::ImageSkia ProfileMenuView::GetSyncIcon() const {
+  Profile* profile = browser()->profile();
+
+  if (!profile->IsRegularProfile())
+    return gfx::ImageSkia();
+
+  if (!IdentityManagerFactory::GetForProfile(profile)->HasPrimaryAccount())
+    return ColoredImageForMenu(kSyncPausedCircleIcon, gfx::kGoogleGrey500);
+
+  const gfx::VectorIcon* icon = nullptr;
+  ui::NativeTheme::ColorId color_id;
+  int unused;
+  switch (
+      sync_ui_util::GetMessagesForAvatarSyncError(profile, &unused, &unused)) {
+    case sync_ui_util::NO_SYNC_ERROR:
+      icon = &kSyncCircleIcon;
+      color_id = ui::NativeTheme::kColorId_AlertSeverityLow;
+      break;
+    case sync_ui_util::AUTH_ERROR:
+      icon = &kSyncPausedCircleIcon;
+      color_id = ui::NativeTheme::kColorId_ProminentButtonColor;
+      break;
+    case sync_ui_util::MANAGED_USER_UNRECOVERABLE_ERROR:
+    case sync_ui_util::UNRECOVERABLE_ERROR:
+    case sync_ui_util::UPGRADE_CLIENT_ERROR:
+    case sync_ui_util::PASSPHRASE_ERROR:
+    case sync_ui_util::TRUSTED_VAULT_KEY_MISSING_FOR_EVERYTHING_ERROR:
+    case sync_ui_util::TRUSTED_VAULT_KEY_MISSING_FOR_PASSWORDS_ERROR:
+    case sync_ui_util::SETTINGS_UNCONFIRMED_ERROR:
+      icon = &kSyncPausedCircleIcon;
+      color_id = ui::NativeTheme::kColorId_AlertSeverityHigh;
+      break;
+  }
+  const SkColor image_color = GetNativeTheme()->GetSystemColor(color_id);
+  return ColoredImageForMenu(*icon, image_color);
+}
+
 base::string16 ProfileMenuView::GetAccessibleWindowTitle() const {
   return l10n_util::GetStringUTF16(
       IDS_PROFILES_PROFILE_BUBBLE_ACCESSIBLE_TITLE);
@@ -353,63 +390,24 @@ void ProfileMenuView::BuildIdentity() {
 
   if (account_info.has_value()) {
     SetIdentityInfo(
-        account_info.value().account_image.AsImageSkia(), GetSyncIcon(),
+        account_info.value().account_image.AsImageSkia(),
         base::UTF8ToUTF16(account_info.value().full_name),
         IsSyncPaused(profile)
             ? l10n_util::GetStringUTF16(IDS_PROFILES_LOCAL_PROFILE_STATE)
             : base::UTF8ToUTF16(account_info.value().email));
   } else {
     SetIdentityInfo(
-        profile_attributes->GetAvatarIcon().AsImageSkia(), GetSyncIcon(),
+        profile_attributes->GetAvatarIcon().AsImageSkia(),
         /*title=*/base::string16(),
         l10n_util::GetStringUTF16(IDS_PROFILES_LOCAL_PROFILE_STATE));
   }
 }
 
 void ProfileMenuView::BuildGuestIdentity() {
-  SetIdentityInfo(profiles::GetGuestAvatar(), GetSyncIcon(),
+  SetIdentityInfo(profiles::GetGuestAvatar(),
                   l10n_util::GetStringUTF16(IDS_GUEST_PROFILE_NAME));
 }
 
-gfx::ImageSkia ProfileMenuView::GetSyncIcon() {
-  Profile* profile = browser()->profile();
-  signin::IdentityManager* identity_manager =
-      IdentityManagerFactory::GetForProfile(profile);
-  ui::NativeTheme* native_theme = ui::NativeTheme::GetInstanceForNativeUi();
-
-  if (!profile->IsRegularProfile())
-    return gfx::ImageSkia();
-
-  if (!identity_manager->HasPrimaryAccount())
-    return ColoredImageForMenu(kSyncPausedCircleIcon, gfx::kGoogleGrey500);
-
-  const gfx::VectorIcon* icon = nullptr;
-  ui::NativeTheme::ColorId color_id;
-  int unused;
-  switch (
-      sync_ui_util::GetMessagesForAvatarSyncError(profile, &unused, &unused)) {
-    case sync_ui_util::NO_SYNC_ERROR:
-      icon = &kSyncCircleIcon;
-      color_id = ui::NativeTheme::kColorId_AlertSeverityLow;
-      break;
-    case sync_ui_util::AUTH_ERROR:
-      icon = &kSyncPausedCircleIcon;
-      color_id = ui::NativeTheme::kColorId_ProminentButtonColor;
-      break;
-    case sync_ui_util::MANAGED_USER_UNRECOVERABLE_ERROR:
-    case sync_ui_util::UNRECOVERABLE_ERROR:
-    case sync_ui_util::UPGRADE_CLIENT_ERROR:
-    case sync_ui_util::PASSPHRASE_ERROR:
-    case sync_ui_util::TRUSTED_VAULT_KEY_MISSING_FOR_EVERYTHING_ERROR:
-    case sync_ui_util::TRUSTED_VAULT_KEY_MISSING_FOR_PASSWORDS_ERROR:
-    case sync_ui_util::SETTINGS_UNCONFIRMED_ERROR:
-      icon = &kSyncPausedCircleIcon;
-      color_id = ui::NativeTheme::kColorId_AlertSeverityHigh;
-      break;
-  }
-  return ColoredImageForMenu(*icon, native_theme->GetSystemColor(color_id));
-}
-
 void ProfileMenuView::BuildAutofillButtons() {
   AddShortcutFeatureButton(
       ImageForMenu(kKeyIcon, kShortcutIconToImageRatio),
@@ -450,7 +448,6 @@ void ProfileMenuView::BuildSyncInfo() {
 
     if (error == sync_ui_util::NO_SYNC_ERROR) {
       SetSyncInfo(
-          GetSyncIcon(),
           /*description=*/base::string16(),
           l10n_util::GetStringUTF16(IDS_PROFILES_OPEN_SYNC_SETTINGS_BUTTON),
           SyncInfoContainerBackgroundState::kNoError,
@@ -470,7 +467,7 @@ void ProfileMenuView::BuildSyncInfo() {
                                      : IDS_SYNC_ERROR_USER_MENU_TITLE;
 
       SetSyncInfo(
-          GetSyncIcon(), l10n_util::GetStringUTF16(description_string_id),
+          l10n_util::GetStringUTF16(description_string_id),
           l10n_util::GetStringUTF16(button_string_id),
           sync_paused ? SyncInfoContainerBackgroundState::kPaused
                       : SyncInfoContainerBackgroundState::kError,
@@ -489,19 +486,18 @@ void ProfileMenuView::BuildSyncInfo() {
 
   if (account_info.has_value()) {
     SetSyncInfo(
-        GetSyncIcon(),
         l10n_util::GetStringUTF16(IDS_PROFILES_DICE_NOT_SYNCING_TITLE),
         l10n_util::GetStringUTF16(IDS_PROFILES_DICE_SIGNIN_BUTTON),
         SyncInfoContainerBackgroundState::kNoPrimaryAccount,
         base::BindRepeating(&ProfileMenuView::OnSigninAccountButtonClicked,
                             base::Unretained(this), account_info.value()));
   } else {
-    SetSyncInfo(/*icon=*/gfx::ImageSkia(),
-                l10n_util::GetStringUTF16(IDS_PROFILES_DICE_SYNC_PROMO),
+    SetSyncInfo(l10n_util::GetStringUTF16(IDS_PROFILES_DICE_SYNC_PROMO),
                 l10n_util::GetStringUTF16(IDS_PROFILES_DICE_SIGNIN_BUTTON),
                 SyncInfoContainerBackgroundState::kNoPrimaryAccount,
                 base::BindRepeating(&ProfileMenuView::OnSigninButtonClicked,
-                                    base::Unretained(this)));
+                                    base::Unretained(this)),
+                /*show_badge=*/false);
   }
 }
 

chrome/browser/ui/views/profiles/profile_menu_view.h

@@ -36,6 +36,7 @@ class ProfileMenuView : public ProfileMenuViewBase {
 
   // ProfileMenuViewBase:
   void BuildMenu() override;
+  gfx::ImageSkia GetSyncIcon() const override;
 
  private:
   friend class ProfileMenuViewExtensionsTest;
@@ -69,7 +70,6 @@ class ProfileMenuView : public ProfileMenuViewBase {
   // Helper methods for building the menu.
   void BuildIdentity();
   void BuildGuestIdentity();
-  gfx::ImageSkia GetSyncIcon();
   void BuildAutofillButtons();
   void BuildSyncInfo();
   void BuildFeatureButtons();

chrome/browser/ui/views/profiles/profile_menu_view_base.cc

@@ -54,6 +54,7 @@ constexpr int kMenuWidth = 288;
 constexpr int kIdentityImageSize = 64;
 constexpr int kMaxImageSize = kIdentityImageSize;
 constexpr int kDefaultVerticalMargin = 8;
+constexpr int kBadgeSize = 16;
 
 // If the bubble is too large to fit on the screen, it still needs to be at
 // least this tall to show one row.
@@ -159,6 +160,81 @@ std::unique_ptr<views::Button> CreateCircularImageButton(
   return button;
 }
 
+// AvatarImageView is used to ensure avatar adornments are kept in sync with
+// current theme colors.
+class AvatarImageView : public views::ImageView {
+ public:
+  AvatarImageView(gfx::ImageSkia avatar_image,
+                  const ProfileMenuViewBase* root_view)
+      : avatar_image_(avatar_image), root_view_(root_view) {
+    SetBorder(views::CreateEmptyBorder(0, 0, kDefaultVerticalMargin, 0));
+  }
+
+  // views::ImageVIew:
+  void OnThemeChanged() override {
+    ImageView::OnThemeChanged();
+    // Fall back on |kUserAccountAvatarIcon| if |image| is empty. This can
+    // happen in tests and when the account image hasn't been fetched yet.
+    constexpr int kBadgePadding = 1;
+    const SkColor icon_color = GetNativeTheme()->GetSystemColor(
+        ui::NativeTheme::kColorId_DefaultIconColor);
+    gfx::ImageSkia sized_avatar_image =
+        avatar_image_.isNull()
+            ? gfx::CreateVectorIcon(kUserAccountAvatarIcon, kIdentityImageSize,
+                                    icon_color)
+            : CropCircle(SizeImage(avatar_image_, kIdentityImageSize));
+
+    const SkColor background_color = GetNativeTheme()->GetSystemColor(
+        ui::NativeTheme::kColorId_BubbleBackground);
+    gfx::ImageSkia sized_badge =
+        AddCircularBackground(SizeImage(root_view_->GetSyncIcon(), kBadgeSize),
+                              background_color, kBadgeSize + 2 * kBadgePadding);
+    gfx::ImageSkia sized_badge_with_shadow =
+        gfx::ImageSkiaOperations::CreateImageWithDropShadow(
+            sized_badge, gfx::ShadowValue::MakeMdShadowValues(/*elevation=*/1,
+                                                              SK_ColorBLACK));
+
+    gfx::ImageSkia badged_image = gfx::ImageSkiaOperations::CreateIconWithBadge(
+        sized_avatar_image, sized_badge_with_shadow);
+    SetImage(badged_image);
+  }
+
+ private:
+  gfx::ImageSkia avatar_image_;
+  const ProfileMenuViewBase* root_view_;
+};
+
+class SyncButton : public HoverButton {
+ public:
+  SyncButton(ProfileMenuViewBase* root_view,
+             const base::string16& clickable_text)
+      : HoverButton(root_view, clickable_text), root_view_(root_view) {}
+
+  // HoverButton:
+  void OnThemeChanged() override {
+    HoverButton::OnThemeChanged();
+    SetImage(STATE_NORMAL, SizeImage(root_view_->GetSyncIcon(), kBadgeSize));
+  }
+
+ private:
+  const ProfileMenuViewBase* root_view_;
+};
+
+class SyncImageView : public views::ImageView {
+ public:
+  explicit SyncImageView(const ProfileMenuViewBase* root_view)
+      : root_view_(root_view) {}
+
+  // views::ImageView:
+  void OnThemeChanged() override {
+    ImageView::OnThemeChanged();
+    SetImage(SizeImage(root_view_->GetSyncIcon(), kBadgeSize));
+  }
+
+ private:
+  const ProfileMenuViewBase* root_view_;
+};
+
 }  // namespace
 
 // ProfileMenuViewBase ---------------------------------------------------------
@@ -236,13 +312,16 @@ ProfileMenuViewBase::~ProfileMenuViewBase() {
   DCHECK(g_profile_bubble_ != this);
 }
 
+gfx::ImageSkia ProfileMenuViewBase::GetSyncIcon() const {
+  return gfx::ImageSkia();
+}
+
 void ProfileMenuViewBase::SetHeading(const base::string16& heading,
                                      const base::string16& tooltip_text,
                                      base::RepeatingClosure action) {
   constexpr int kInsidePadding = 8;
-  const SkColor kBackgroundColor =
-      ui::NativeTheme::GetInstanceForNativeUi()->GetSystemColor(
-          ui::NativeTheme::kColorId_HighlightedMenuItemBackgroundColor);
+  const SkColor kBackgroundColor = GetNativeTheme()->GetSystemColor(
+      ui::NativeTheme::kColorId_HighlightedMenuItemBackgroundColor);
 
   heading_container_->RemoveAllChildViews(/*delete_children=*/true);
   heading_container_->SetLayoutManager(std::make_unique<views::FillLayout>());
@@ -260,17 +339,11 @@ void ProfileMenuViewBase::SetHeading(const base::string16& heading,
 }
 
 void ProfileMenuViewBase::SetIdentityInfo(const gfx::ImageSkia& image,
-                                          const gfx::ImageSkia& badge,
                                           const base::string16& title,
                                           const base::string16& subtitle) {
   constexpr int kTopMargin = kMenuEdgeMargin;
   constexpr int kBottomMargin = kDefaultVerticalMargin;
   constexpr int kHorizontalMargin = kMenuEdgeMargin;
-  constexpr int kImageBottomMargin = kDefaultVerticalMargin;
-  constexpr int kBadgeSize = 16;
-  constexpr int kBadgePadding = 1;
-  const SkColor kBadgeBackgroundColor = GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_BubbleBackground);
 
   identity_info_container_->RemoveAllChildViews(/*delete_children=*/true);
   identity_info_container_->SetLayoutManager(
@@ -279,27 +352,8 @@ void ProfileMenuViewBase::SetIdentityInfo(const gfx::ImageSkia& image,
                       gfx::Insets(kTopMargin, kHorizontalMargin, kBottomMargin,
                                   kHorizontalMargin)));
 
-  views::ImageView* image_view = identity_info_container_->AddChildView(
-      std::make_unique<views::ImageView>());
-  // Fall back on |kUserAccountAvatarIcon| if |image| is empty. This can happen
-  // in tests and when the account image hasn't been fetched yet.
-  gfx::ImageSkia sized_image =
-      image.isNull()
-          ? gfx::CreateVectorIcon(kUserAccountAvatarIcon, kIdentityImageSize,
-                                  GetDefaultIconColor())
-          : CropCircle(SizeImage(image, kIdentityImageSize));
-  gfx::ImageSkia sized_badge =
-      AddCircularBackground(SizeImage(badge, kBadgeSize), kBadgeBackgroundColor,
-                            kBadgeSize + 2 * kBadgePadding);
-  gfx::ImageSkia sized_badge_with_shadow =
-      gfx::ImageSkiaOperations::CreateImageWithDropShadow(
-          sized_badge,
-          gfx::ShadowValue::MakeMdShadowValues(/*elevation=*/1, SK_ColorBLACK));
-
-  gfx::ImageSkia badged_image = gfx::ImageSkiaOperations::CreateIconWithBadge(
-      sized_image, sized_badge_with_shadow);
-  image_view->SetImage(badged_image);
-  image_view->SetBorder(views::CreateEmptyBorder(0, 0, kImageBottomMargin, 0));
+  identity_info_container_->AddChildView(
+      std::make_unique<AvatarImageView>(image, this));
 
   if (!title.empty()) {
     identity_info_container_->AddChildView(std::make_unique<views::Label>(
@@ -313,12 +367,11 @@ void ProfileMenuViewBase::SetIdentityInfo(const gfx::ImageSkia& image,
 }
 
 void ProfileMenuViewBase::SetSyncInfo(
-    const gfx::ImageSkia& icon,
     const base::string16& description,
     const base::string16& clickable_text,
     SyncInfoContainerBackgroundState sync_background_state,
-    base::RepeatingClosure action) {
-  constexpr int kIconSize = 16;
+    base::RepeatingClosure action,
+    bool show_badge) {
   const int kDescriptionIconSpacing =
       ChromeLayoutProvider::Get()->GetDistanceMetric(
           views::DISTANCE_RELATED_LABEL_HORIZONTAL);
@@ -335,17 +388,18 @@ void ProfileMenuViewBase::SetSyncInfo(
       views::BoxLayout::Orientation::kVertical, gfx::Insets(), kInsidePadding));
 
   if (description.empty()) {
-    views::Button* button =
-        sync_info_container_->AddChildView(std::make_unique<HoverButton>(
-            this, SizeImage(icon, kIconSize), clickable_text));
-    RegisterClickAction(button, std::move(action));
+    views::Button* sync_button = sync_info_container_->AddChildView(
+        std::make_unique<SyncButton>(this, clickable_text));
+    RegisterClickAction(sync_button, std::move(action));
     return;
   }
 
+  const SkColor border_color = GetNativeTheme()->GetSystemColor(
+      ui::NativeTheme::kColorId_MenuSeparatorColor);
   // Add padding, rounded border and margins.
   sync_info_container_->SetBorder(views::CreatePaddedBorder(
       views::CreateRoundedRectBorder(kBorderThickness, kBorderCornerRadius,
-                                     GetDefaultSeparatorColor()),
+                                     border_color),
       gfx::Insets(kInsidePadding)));
   sync_info_container_->SetProperty(
       views::kMarginsKey, gfx::Insets(kDefaultVerticalMargin, kMenuEdgeMargin));
@@ -359,14 +413,12 @@ void ProfileMenuViewBase::SetSyncInfo(
               views::BoxLayout::Orientation::kHorizontal, gfx::Insets(),
               kDescriptionIconSpacing));
 
-  if (icon.isNull()) {
+  if (show_badge) {
+    description_container->AddChildView(std::make_unique<SyncImageView>(this));
+  } else {
     // If there is no image, the description is centered.
     description_layout->set_main_axis_alignment(
         views::BoxLayout::MainAxisAlignment::kCenter);
-  } else {
-    views::ImageView* icon_view = description_container->AddChildView(
-        std::make_unique<views::ImageView>());
-    icon_view->SetImage(SizeImage(icon, kIconSize));
   }
 
   views::Label* label = description_container->AddChildView(
@@ -513,8 +565,9 @@ void ProfileMenuViewBase::AddProfileManagementFeatureButton(
   RegisterClickAction(button, std::move(action));
 }
 
-gfx::ImageSkia ProfileMenuViewBase::ImageForMenu(const gfx::VectorIcon& icon,
-                                                 float icon_to_image_ratio) {
+gfx::ImageSkia ProfileMenuViewBase::ImageForMenu(
+    const gfx::VectorIcon& icon,
+    float icon_to_image_ratio) const {
   const int padding =
       static_cast<int>(kMaxImageSize * (1.0 - icon_to_image_ratio) / 2.0);
 
@@ -525,7 +578,7 @@ gfx::ImageSkia ProfileMenuViewBase::ImageForMenu(const gfx::VectorIcon& icon,
 
 gfx::ImageSkia ProfileMenuViewBase::ColoredImageForMenu(
     const gfx::VectorIcon& icon,
-    SkColor color) {
+    SkColor color) const {
   return gfx::CreateVectorIcon(icon, kMaxImageSize, color);
 }
 

chrome/browser/ui/views/profiles/profile_menu_view_base.h

@@ -92,19 +92,21 @@ class ProfileMenuViewBase : public content::WebContentsDelegate,
   // This method is called once to add all menu items.
   virtual void BuildMenu() = 0;
 
+  // Override to supply a sync icon for the profile menu.
+  virtual gfx::ImageSkia GetSyncIcon() const;
+
   // API to build the profile menu.
   void SetHeading(const base::string16& heading,
                   const base::string16& tooltip_text,
                   base::RepeatingClosure action);
   void SetIdentityInfo(const gfx::ImageSkia& image,
-                       const gfx::ImageSkia& badge,
                        const base::string16& title,
                        const base::string16& subtitle = base::string16());
-  void SetSyncInfo(const gfx::ImageSkia& icon,
-                   const base::string16& description,
+  void SetSyncInfo(const base::string16& description,
                    const base::string16& clickable_text,
                    SyncInfoContainerBackgroundState background_state,
-                   base::RepeatingClosure action);
+                   base::RepeatingClosure action,
+                   bool show_badge = true);
   void AddShortcutFeatureButton(const gfx::ImageSkia& icon,
                                 const base::string16& text,
                                 base::RepeatingClosure action);
@@ -122,13 +124,14 @@ class ProfileMenuViewBase : public content::WebContentsDelegate,
   void AddProfileManagementFeatureButton(const gfx::ImageSkia& icon,
                                          const base::string16& text,
                                          base::RepeatingClosure action);
+
   // 0 < |icon_to_image_ratio| <= 1 is the size ratio of |icon| in the returned
   // image. E.g. a value of 0.8 means that |icon| only takes up 80% of the
   // returned image, with the rest being padding around it.
   gfx::ImageSkia ImageForMenu(const gfx::VectorIcon& icon,
-                              float icon_to_image_ratio = 1.0f);
+                              float icon_to_image_ratio = 1.0f) const;
   gfx::ImageSkia ColoredImageForMenu(const gfx::VectorIcon& icon,
-                                     SkColor color);
+                                     SkColor color) const;
   // Should be called inside each button/link action.
   void RecordClick(ActionableItem item);