Create and use ThemedVectorIcon for menu icons.

Bug: 1056944
Change-Id: Ia5d93433f1901156c5585c9a712e340c39bb173c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2112578
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755003}

chrome/browser/ui/views/menu_item_view_interactive_uitest.cc

@@ -5,6 +5,7 @@
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/views/menu_test_base.h"
+#include "ui/native_theme/themed_vector_icon.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/submenu_view.h"
 
@@ -91,8 +92,8 @@ class MenuItemViewTestInsert : public MenuTestBase {
 
     inserted_item_ = menu()->AddMenuItemAt(
         INSERT_INDEX, 1000, ASCIIToUTF16("inserted item"), base::string16(),
-        nullptr, gfx::ImageSkia(), nullptr, views::MenuItemView::Type::kNormal,
-        ui::NORMAL_SEPARATOR);
+        ui::ThemedVectorIcon(), gfx::ImageSkia(), ui::ThemedVectorIcon(),
+        views::MenuItemView::Type::kNormal, ui::NORMAL_SEPARATOR);
     ASSERT_TRUE(inserted_item_);
     menu()->ChildrenChanged();
 
@@ -186,8 +187,8 @@ class MenuItemViewTestInsertWithSubmenu : public MenuTestBase {
   void Step2() {
     inserted_item_ = menu()->AddMenuItemAt(
         INSERT_INDEX, 1000, ASCIIToUTF16("inserted item"), base::string16(),
-        nullptr, gfx::ImageSkia(), nullptr, views::MenuItemView::Type::kNormal,
-        ui::NORMAL_SEPARATOR);
+        ui::ThemedVectorIcon(), gfx::ImageSkia(), ui::ThemedVectorIcon(),
+        views::MenuItemView::Type::kNormal, ui::NORMAL_SEPARATOR);
     ASSERT_TRUE(inserted_item_);
     menu()->ChildrenChanged();
 

ui/native_theme/BUILD.gn

@@ -28,6 +28,8 @@ jumbo_component("native_theme") {
     "native_theme_features.h",
     "native_theme_observer.cc",
     "native_theme_observer.h",
+    "themed_vector_icon.cc",
+    "themed_vector_icon.h",
   ]
 
   if (is_android) {

ui/native_theme/themed_vector_icon.cc

+// Copyright 2020 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 "ui/native_theme/themed_vector_icon.h"
+
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/vector_icon_types.h"
+
+namespace ui {
+
+ThemedVectorIcon::ThemedVectorIcon() = default;
+
+ThemedVectorIcon::ThemedVectorIcon(const gfx::VectorIcon* icon,
+                                   NativeTheme::ColorId color_id)
+    : icon_(icon), color_id_(color_id) {}
+
+ThemedVectorIcon::ThemedVectorIcon(const gfx::VectorIcon* icon, SkColor color)
+    : icon_(icon), color_(color) {}
+
+ThemedVectorIcon::ThemedVectorIcon(const ThemedVectorIcon& other) = default;
+
+const gfx::ImageSkia ThemedVectorIcon::GetImageSkia(NativeTheme* theme) const {
+  DCHECK(!empty());
+  return CreateVectorIcon(*icon_, GetColor(theme));
+}
+
+const gfx::ImageSkia ThemedVectorIcon::GetImageSkia(NativeTheme* theme,
+                                                    int icon_size) const {
+  DCHECK(!empty());
+  return CreateVectorIcon(*icon_, icon_size, GetColor(theme));
+}
+
+const gfx::ImageSkia ThemedVectorIcon::GetImageSkia(SkColor color) const {
+  DCHECK(!empty());
+  return CreateVectorIcon(*icon_, color);
+}
+
+SkColor ThemedVectorIcon::GetColor(NativeTheme* theme) const {
+  DCHECK(color_id_ || color_);
+  return color_id_ ? theme->GetSystemColor(color_id_.value()) : color_.value();
+}
+
+}  // namespace ui
\ No newline at end of file

ui/native_theme/themed_vector_icon.h

+// Copyright 2020 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 UI_NATIVE_THEME_THEMED_VECTOR_ICON_H_
+#define UI_NATIVE_THEME_THEMED_VECTOR_ICON_H_
+
+#include "base/optional.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/native_theme/native_theme.h"
+#include "ui/native_theme/native_theme_export.h"
+
+namespace gfx {
+class ImageSkia;
+struct VectorIcon;
+}  // namespace gfx
+
+namespace ui {
+
+class NATIVE_THEME_EXPORT ThemedVectorIcon {
+ public:
+  ThemedVectorIcon();
+  explicit ThemedVectorIcon(
+      const gfx::VectorIcon* icon,
+      NativeTheme::ColorId color_id = NativeTheme::kColorId_DefaultIconColor);
+  // TODO (kylixrd): Remove this once all the hard-coded uses of color are
+  // removed.
+  ThemedVectorIcon(const gfx::VectorIcon* icon, SkColor color);
+  ThemedVectorIcon(const ThemedVectorIcon& other);
+
+  void clear() { icon_ = nullptr; }
+  bool empty() const { return !icon_; }
+  const gfx::ImageSkia GetImageSkia(NativeTheme* theme) const;
+  const gfx::ImageSkia GetImageSkia(NativeTheme* theme, int icon_size) const;
+  const gfx::ImageSkia GetImageSkia(SkColor color) const;
+
+ private:
+  SkColor GetColor(NativeTheme* theme) const;
+
+  const gfx::VectorIcon* icon_ = nullptr;
+  base::Optional<NativeTheme::ColorId> color_id_;
+  base::Optional<SkColor> color_;
+};
+
+}  // namespace ui
+
+#endif  // UI_NATIVE_THEME_THEMED_VECTOR_ICON_H_

ui/views/controls/menu/menu_item_view.cc

@@ -262,9 +262,9 @@ MenuItemView* MenuItemView::AddMenuItemAt(
     int item_id,
     const base::string16& label,
     const base::string16& minor_text,
-    const gfx::VectorIcon* minor_icon,
+    const ui::ThemedVectorIcon& minor_icon,
     const gfx::ImageSkia& icon,
-    const gfx::VectorIcon* vector_icon,
+    const ui::ThemedVectorIcon& vector_icon,
     Type type,
     ui::MenuSeparatorType separator_style) {
   DCHECK_NE(type, Type::kEmpty);
@@ -284,7 +284,7 @@ MenuItemView* MenuItemView::AddMenuItemAt(
     item->SetTitle(label);
   item->SetMinorText(minor_text);
   item->SetMinorIcon(minor_icon);
-  if (vector_icon) {
+  if (!vector_icon.empty()) {
     DCHECK(icon.isNull());
     item->SetIcon(vector_icon);
   }
@@ -337,8 +337,10 @@ void MenuItemView::AppendSeparator() {
 
 void MenuItemView::AddSeparatorAt(int index) {
   AddMenuItemAt(index, /*item_id=*/0, /*label=*/base::string16(),
-                /*minor_text=*/base::string16(), /*minor_icon=*/nullptr,
-                /*icon=*/gfx::ImageSkia(), /*vector_icon=*/nullptr,
+                /*minor_text=*/base::string16(),
+                /*minor_icon=*/ui::ThemedVectorIcon(),
+                /*icon=*/gfx::ImageSkia(),
+                /*vector_icon=*/ui::ThemedVectorIcon(),
                 /*type=*/Type::kSeparator,
                 /*separator_style=*/ui::NORMAL_SEPARATOR);
 }
@@ -348,8 +350,9 @@ MenuItemView* MenuItemView::AppendMenuItemImpl(int item_id,
                                                const gfx::ImageSkia& icon,
                                                Type type) {
   const int index = submenu_ ? int{submenu_->children().size()} : 0;
-  return AddMenuItemAt(index, item_id, label, base::string16(), nullptr, icon,
-                       nullptr, type, ui::NORMAL_SEPARATOR);
+  return AddMenuItemAt(index, item_id, label, base::string16(),
+                       ui::ThemedVectorIcon(), icon, ui::ThemedVectorIcon(),
+                       type, ui::NORMAL_SEPARATOR);
 }
 
 SubmenuView* MenuItemView::CreateSubmenu() {
@@ -385,7 +388,7 @@ void MenuItemView::SetMinorText(const base::string16& minor_text) {
   invalidate_dimensions();  // Triggers preferred size recalculation.
 }
 
-void MenuItemView::SetMinorIcon(const gfx::VectorIcon* minor_icon) {
+void MenuItemView::SetMinorIcon(const ui::ThemedVectorIcon& minor_icon) {
   minor_icon_ = minor_icon;
   invalidate_dimensions();  // Triggers preferred size recalculation.
 }
@@ -421,48 +424,44 @@ void MenuItemView::SetIcon(const gfx::ImageSkia& icon, int item_id) {
 }
 
 void MenuItemView::SetIcon(const gfx::ImageSkia& icon) {
-  vector_icon_ = nullptr;
+  vector_icon_.clear();
 
   if (icon.isNull()) {
     SetIconView(nullptr);
     return;
   }
 
-  ImageView* icon_view = new ImageView();
+  auto icon_view = std::make_unique<ImageView>();
   icon_view->SetImage(&icon);
-  SetIconView(icon_view);
+  SetIconView(std::move(icon_view));
 }
 
-void MenuItemView::SetIcon(const gfx::VectorIcon* icon) {
+void MenuItemView::SetIcon(const ui::ThemedVectorIcon& icon) {
   vector_icon_ = icon;
 }
 
 void MenuItemView::UpdateIconViewFromVectorIconAndTheme() {
-  if (!vector_icon_)
+  if (vector_icon_.empty())
     return;
 
   if (!icon_view_)
-    SetIconView(new ImageView());
+    SetIconView(std::make_unique<ImageView>());
 
   const bool use_touchable_layout =
       GetMenuController() && GetMenuController()->use_touchable_layout();
   const int icon_size = use_touchable_layout ? 20 : 16;
-  icon_view_->SetImage(
-      gfx::CreateVectorIcon(*vector_icon_, icon_size,
-                            GetNativeTheme()->GetSystemColor(
-                                ui::NativeTheme::kColorId_DefaultIconColor)));
+  icon_view_->SetImage(vector_icon_.GetImageSkia(GetNativeTheme(), icon_size));
 }
 
-void MenuItemView::SetIconView(ImageView* icon_view) {
+void MenuItemView::SetIconView(std::unique_ptr<ImageView> icon_view) {
   if (icon_view_) {
-    RemoveChildView(icon_view_);
-    delete icon_view_;
+    RemoveChildViewT(icon_view_);
     icon_view_ = nullptr;
   }
-  if (icon_view) {
-    AddChildView(icon_view);
-    icon_view_ = icon_view;
-  }
+
+  if (icon_view)
+    icon_view_ = AddChildView(std::move(icon_view));
+
   InvalidateLayout();
   SchedulePaint();
 }
@@ -1011,8 +1010,8 @@ void MenuItemView::PaintMinorIconAndText(
     gfx::Canvas* canvas,
     const MenuDelegate::LabelStyle& style) {
   base::string16 minor_text = GetMinorText();
-  const gfx::VectorIcon* minor_icon = GetMinorIcon();
-  if (minor_text.empty() && !minor_icon)
+  const ui::ThemedVectorIcon minor_icon = GetMinorIcon();
+  if (minor_text.empty() && minor_icon.empty())
     return;
 
   int available_height = height() - GetTopMargin() - GetBottomMargin();
@@ -1039,8 +1038,8 @@ void MenuItemView::PaintMinorIconAndText(
     render_text->Draw(canvas);
   }
 
-  if (minor_icon) {
-    gfx::ImageSkia image = CreateVectorIcon(*minor_icon, style.foreground);
+  if (!minor_icon.empty()) {
+    gfx::ImageSkia image = minor_icon.GetImageSkia(style.foreground);
 
     int image_x = GetMirroredRect(minor_text_bounds).right() -
                   render_text->GetContentWidth() -
@@ -1266,7 +1265,7 @@ base::string16 MenuItemView::GetMinorText() const {
   return minor_text_;
 }
 
-const gfx::VectorIcon* MenuItemView::GetMinorIcon() const {
+ui::ThemedVectorIcon MenuItemView::GetMinorIcon() const {
   return minor_icon_;
 }
 

ui/views/controls/menu/menu_item_view.h

@@ -18,20 +18,15 @@
 #include "ui/base/models/menu_separator_types.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/image/image_skia.h"
+#include "ui/native_theme/themed_vector_icon.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/controls/menu/menu_types.h"
 #include "ui/views/view.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
-
-#include "ui/native_theme/native_theme.h"
 #endif
 
-namespace gfx {
-struct VectorIcon;
-}
-
 namespace views {
 
 namespace internal {
@@ -154,9 +149,9 @@ class VIEWS_EXPORT MenuItemView : public View {
                               int item_id,
                               const base::string16& label,
                               const base::string16& minor_text,
-                              const gfx::VectorIcon* minor_icon,
+                              const ui::ThemedVectorIcon& minor_icon,
                               const gfx::ImageSkia& icon,
-                              const gfx::VectorIcon* vector_icon,
+                              const ui::ThemedVectorIcon& vector_icon,
                               Type type,
                               ui::MenuSeparatorType separator_style);
 
@@ -223,7 +218,7 @@ class VIEWS_EXPORT MenuItemView : public View {
   void SetMinorText(const base::string16& minor_text);
 
   // Sets the minor icon.
-  void SetMinorIcon(const gfx::VectorIcon* minor_icon);
+  void SetMinorIcon(const ui::ThemedVectorIcon& minor_icon);
 
   // Returns the type of this menu.
   const Type& GetType() const { return type_; }
@@ -253,12 +248,13 @@ class VIEWS_EXPORT MenuItemView : public View {
   // Sets the icon of this menu item.
   void SetIcon(const gfx::ImageSkia& icon);
 
-  // Sets the icon as a vector icon which gets its color from the NativeTheme.
-  void SetIcon(const gfx::VectorIcon* icon);
+  // Sets the icon as a vector icon which gets its color from the NativeTheme or
+  // the included color.
+  void SetIcon(const ui::ThemedVectorIcon& icon);
 
   // Sets the view used to render the icon. This clobbers any icon set via
   // SetIcon(). MenuItemView takes ownership of |icon_view|.
-  void SetIconView(ImageView* icon_view);
+  void SetIconView(std::unique_ptr<ImageView> icon_view);
 
   void UpdateIconViewFromVectorIconAndTheme();
 
@@ -428,7 +424,7 @@ class VIEWS_EXPORT MenuItemView : public View {
   base::string16 GetMinorText() const;
 
   // Returns the icon that should be displayed to the left of the minor text.
-  const gfx::VectorIcon* GetMinorIcon() const;
+  ui::ThemedVectorIcon GetMinorIcon() const;
 
   // Returns the text color for the current state.  |minor| specifies if the
   // minor text or the normal text is desired.
@@ -519,11 +515,11 @@ class VIEWS_EXPORT MenuItemView : public View {
   base::string16 minor_text_;
 
   // Minor icon.
-  const gfx::VectorIcon* minor_icon_ = nullptr;
+  ui::ThemedVectorIcon minor_icon_;
 
   // The icon used for |icon_view_| when a vector icon has been set instead of a
   // gfx::Image.
-  const gfx::VectorIcon* vector_icon_ = nullptr;
+  ui::ThemedVectorIcon vector_icon_;
 
   // Does the title have a mnemonic? Only useful on the root menu item.
   bool has_mnemonics_ = false;

ui/views/controls/menu/menu_item_view_unittest.cc

@@ -13,6 +13,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/compositor/canvas_painter.h"
 #include "ui/gfx/geometry/insets.h"
+#include "ui/native_theme/themed_vector_icon.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/controls/menu/submenu_view.h"
 #include "ui/views/controls/menu/test_menu_item_view.h"
@@ -322,16 +323,18 @@ class MenuItemViewPaintUnitTest : public ViewsTestBase {
 // Provides assertion coverage for painting minor text and icons.
 TEST_F(MenuItemViewPaintUnitTest, MinorTextAndIconAssertionCoverage) {
   auto AddItem = [this](auto label, auto minor_label, auto minor_icon) {
-    menu_item_view()->AddMenuItemAt(0, 1000, base::ASCIIToUTF16(label),
-                                    minor_label, minor_icon, gfx::ImageSkia(),
-                                    nullptr, views::MenuItemView::Type::kNormal,
-                                    ui::NORMAL_SEPARATOR);
+    menu_item_view()->AddMenuItemAt(
+        0, 1000, base::ASCIIToUTF16(label), minor_label, minor_icon,
+        gfx::ImageSkia(), ui::ThemedVectorIcon(),
+        views::MenuItemView::Type::kNormal, ui::NORMAL_SEPARATOR);
   };
-  AddItem("No minor content", base::string16(), nullptr);
-  AddItem("Minor text only", base::ASCIIToUTF16("minor text"), nullptr);
-  AddItem("Minor icon only", base::string16(), &views::kMenuCheckIcon);
+  AddItem("No minor content", base::string16(), ui::ThemedVectorIcon());
+  AddItem("Minor text only", base::ASCIIToUTF16("minor text"),
+          ui::ThemedVectorIcon());
+  AddItem("Minor icon only", base::string16(),
+          ui::ThemedVectorIcon(&views::kMenuCheckIcon));
   AddItem("Minor text and icon", base::ASCIIToUTF16("minor text"),
-          &views::kMenuCheckIcon);
+          ui::ThemedVectorIcon(&views::kMenuCheckIcon));
 
   menu_runner()->RunMenuAt(widget(), nullptr, gfx::Rect(),
                            MenuAnchorPosition::kTopLeft,

ui/views/controls/menu/menu_model_adapter.cc

@@ -99,8 +99,8 @@ MenuItemView* MenuModelAdapter::AddMenuItemFromModelAt(ui::MenuModel* model,
 
   if (*type == MenuItemView::Type::kSeparator) {
     return menu->AddMenuItemAt(menu_index, item_id, base::string16(),
-                               base::string16(), nullptr, gfx::ImageSkia(),
-                               nullptr, *type,
+                               base::string16(), ui::ThemedVectorIcon(),
+                               gfx::ImageSkia(), ui::ThemedVectorIcon(), *type,
                                model->GetSeparatorTypeAt(model_index));
   }
 
@@ -108,10 +108,12 @@ MenuItemView* MenuModelAdapter::AddMenuItemFromModelAt(ui::MenuModel* model,
   model->GetIconAt(model_index, &icon);
   return menu->AddMenuItemAt(
       menu_index, item_id, model->GetLabelAt(model_index),
-      model->GetMinorTextAt(model_index), model->GetMinorIconAt(model_index),
+      model->GetMinorTextAt(model_index),
+      ui::ThemedVectorIcon(model->GetMinorIconAt(model_index)),
       icon.IsEmpty() ? gfx::ImageSkia() : *icon.ToImageSkia(),
-      icon.IsEmpty() ? model->GetVectorIconAt(model_index) : nullptr, *type,
-      ui::NORMAL_SEPARATOR);
+      icon.IsEmpty() ? ui::ThemedVectorIcon(model->GetVectorIconAt(model_index))
+                     : ui::ThemedVectorIcon(),
+      *type, ui::NORMAL_SEPARATOR);
 }
 
 // Static.