Fix icon size for Sharing context menu entries

Before crrev.com/c/2232978 the icon size was hardcoded in MenuItemView,
but that CL changed it to respect the incoming vector icon size. We
can't modify the vector source so we need to set a size when using it.
This also cleans up the duplicated *WithIcon methods and unifies them to
a single one taking a ui::ImageModel instead.

Bug: 1109643
Change-Id: Ib78f683ffee672668c79397122a0e3f5725f58f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2369181Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801441}

chrome/browser/chromeos/arc/intent_helper/open_with_menu.cc

@@ -13,6 +13,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/context_menu_params.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/image_model.h"
 
 namespace arc {
 
@@ -101,7 +102,8 @@ void OpenWithMenu::ModelChanged(const std::vector<LinkHandlerInfo>& handlers) {
           IDS_CONTENT_CONTEXT_OPEN_WITH_APP, it->second.name);
       proxy_->UpdateMenuItem(command_id, true, false, label);
       if (!it->second.icon.IsEmpty())
-        proxy_->UpdateMenuIcon(command_id, it->second.icon);
+        proxy_->UpdateMenuIcon(command_id,
+                               ui::ImageModel::FromImage(it->second.icon));
     }
   }
 }

chrome/browser/chromeos/arc/intent_helper/start_smart_selection_action_menu.cc

@@ -25,6 +25,7 @@
 #include "components/arc/session/arc_bridge_service.h"
 #include "components/renderer_context_menu/render_view_context_menu_proxy.h"
 #include "content/public/browser/context_menu_params.h"
+#include "ui/base/models/image_model.h"
 #include "ui/gfx/image/image_skia_operations.h"
 
 namespace arc {
@@ -179,7 +180,7 @@ void StartSmartSelectionActionMenu::UpdateMenuIcon(
 
 void StartSmartSelectionActionMenu::SetMenuIcon(int command_id,
                                                 const gfx::ImageSkia& image) {
-  proxy_->UpdateMenuIcon(command_id, gfx::Image(image));
+  proxy_->UpdateMenuIcon(command_id, ui::ImageModel::FromImageSkia(image));
 }
 
 }  // namespace arc

chrome/browser/download/download_shelf_context_menu.cc

@@ -413,7 +413,8 @@ void DownloadShelfContextMenu::AddAutoOpenToMenu(ui::SimpleMenuModel* menu) {
         DownloadCommands::ALWAYS_OPEN_TYPE,
         GetLabelForCommandId(DownloadCommands::ALWAYS_OPEN_TYPE),
         ui::ImageModel::FromVectorIcon(vector_icons::kBusinessIcon,
-                                       gfx::kChromeIconGrey, 16));
+                                       gfx::kChromeIconGrey,
+                                       ui::SimpleMenuModel::kDefaultIconSize));
   } else {
     menu->AddCheckItem(
         DownloadCommands::ALWAYS_OPEN_TYPE,

chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc

@@ -63,25 +63,17 @@ void MockRenderViewContextMenu::AddMenuItem(int command_id,
 void MockRenderViewContextMenu::AddMenuItemWithIcon(
     int command_id,
     const base::string16& title,
-    const gfx::ImageSkia& image) {
+    const ui::ImageModel& icon) {
   MockMenuItem item;
   item.command_id = command_id;
   item.enabled = observer_->IsCommandIdEnabled(command_id);
   item.checked = false;
   item.hidden = false;
   item.title = title;
-  item.icon = gfx::Image(image);
+  item.icon = icon;
   items_.push_back(item);
 }
 
-void MockRenderViewContextMenu::AddMenuItemWithIcon(
-    int command_id,
-    const base::string16& title,
-    const gfx::VectorIcon& icon) {
-  AddMenuItemWithIcon(command_id, title,
-                      gfx::CreateVectorIcon(icon, gfx::kPlaceholderColor));
-}
-
 void MockRenderViewContextMenu::AddCheckItem(int command_id,
                                              const base::string16& title) {
   MockMenuItem item;
@@ -120,29 +112,19 @@ void MockRenderViewContextMenu::AddSubMenuWithStringIdAndIcon(
     int command_id,
     int message_id,
     ui::MenuModel* model,
-    const gfx::ImageSkia& image) {
+    const ui::ImageModel& icon) {
   MockMenuItem item;
   item.command_id = command_id;
   item.enabled = observer_->IsCommandIdEnabled(command_id);
   item.checked = observer_->IsCommandIdChecked(command_id);
   item.hidden = false;
   item.title = l10n_util::GetStringUTF16(message_id);
-  item.icon = gfx::Image(image);
+  item.icon = icon;
   items_.push_back(item);
 
   AppendSubMenuItems(model);
 }
 
-void MockRenderViewContextMenu::AddSubMenuWithStringIdAndIcon(
-    int command_id,
-    int message_id,
-    ui::MenuModel* model,
-    const gfx::VectorIcon& icon) {
-  AddSubMenuWithStringIdAndIcon(
-      command_id, message_id, model,
-      gfx::CreateVectorIcon(icon, gfx::kPlaceholderColor));
-}
-
 void MockRenderViewContextMenu::AppendSubMenuItems(ui::MenuModel* model) {
   // Add items in the submenu |model| to |items_| so that the items can be
   // updated later via the RenderViewContextMenuProxy interface.
@@ -180,10 +162,10 @@ void MockRenderViewContextMenu::UpdateMenuItem(int command_id,
 }
 
 void MockRenderViewContextMenu::UpdateMenuIcon(int command_id,
-                                               const gfx::Image& image) {
+                                               const ui::ImageModel& icon) {
   for (auto& item : items_) {
     if (item.command_id == command_id) {
-      item.icon = image;
+      item.icon = icon;
       return;
     }
   }

chrome/browser/renderer_context_menu/mock_render_view_context_menu.h

@@ -12,8 +12,8 @@
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "components/renderer_context_menu/render_view_context_menu_proxy.h"
+#include "ui/base/models/image_model.h"
 #include "ui/base/models/simple_menu_model.h"
-#include "ui/gfx/image/image.h"
 
 class PrefService;
 class Profile;
@@ -39,7 +39,7 @@ class MockRenderViewContextMenu : public ui::SimpleMenuModel::Delegate,
     bool checked;
     bool hidden;
     base::string16 title;
-    gfx::Image icon;
+    ui::ImageModel icon;
   };
 
   explicit MockRenderViewContextMenu(bool incognito);
@@ -54,10 +54,7 @@ class MockRenderViewContextMenu : public ui::SimpleMenuModel::Delegate,
   void AddMenuItem(int command_id, const base::string16& title) override;
   void AddMenuItemWithIcon(int command_id,
                            const base::string16& title,
-                           const gfx::ImageSkia& image) override;
-  void AddMenuItemWithIcon(int command_id,
-                           const base::string16& title,
-                           const gfx::VectorIcon& icon) override;
+                           const ui::ImageModel& icon) override;
   void AddCheckItem(int command_id, const base::string16& title) override;
   void AddSeparator() override;
   void AddSubMenu(int command_id,
@@ -66,16 +63,12 @@ class MockRenderViewContextMenu : public ui::SimpleMenuModel::Delegate,
   void AddSubMenuWithStringIdAndIcon(int command_id,
                                      int message_id,
                                      ui::MenuModel* model,
-                                     const gfx::ImageSkia& image) override;
-  void AddSubMenuWithStringIdAndIcon(int command_id,
-                                     int message_id,
-                                     ui::MenuModel* model,
-                                     const gfx::VectorIcon& icon) override;
+                                     const ui::ImageModel& icon) override;
   void UpdateMenuItem(int command_id,
                       bool enabled,
                       bool hidden,
                       const base::string16& title) override;
-  void UpdateMenuIcon(int command_id, const gfx::Image& image) override;
+  void UpdateMenuIcon(int command_id, const ui::ImageModel& icon) override;
   void RemoveMenuItem(int command_id) override;
   void RemoveAdjacentSeparators() override;
   void AddSpellCheckServiceItem(bool is_checked) override;

chrome/browser/renderer_context_menu/quick_answers_menu_observer.cc

@@ -28,6 +28,8 @@
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/image_model.h"
+#include "ui/base/models/simple_menu_model.h"
 #include "ui/gfx/text_constants.h"
 #include "ui/gfx/text_elider.h"
 
@@ -113,8 +115,10 @@ void QuickAnswersMenuObserver::InitMenu(
   // TODO(llin): Update the menu item after finalizing on the design.
   auto truncated_text = TruncateString(selected_text);
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)
-  proxy_->AddMenuItemWithIcon(IDC_CONTENT_CONTEXT_QUICK_ANSWERS_INLINE_QUERY,
-                              truncated_text, kAssistantIcon);
+  proxy_->AddMenuItemWithIcon(
+      IDC_CONTENT_CONTEXT_QUICK_ANSWERS_INLINE_QUERY, truncated_text,
+      ui::ImageModel::FromVectorIcon(kAssistantIcon, /*color_id=*/-1,
+                                     ui::SimpleMenuModel::kDefaultIconSize));
 #else
   proxy_->AddMenuItem(IDC_CONTENT_CONTEXT_QUICK_ANSWERS_INLINE_QUERY,
                       truncated_text);

chrome/browser/sharing/click_to_call/click_to_call_context_menu_observer.cc

@@ -15,8 +15,8 @@
 #include "chrome/browser/sharing/sharing_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/sync_device_info/device_info.h"
-#include "components/vector_icons/vector_icons.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/image_model.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
 
@@ -78,7 +78,9 @@ void ClickToCallContextMenuObserver::BuildMenu(
         l10n_util::GetStringFUTF16(
             IDS_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_SINGLE_DEVICE,
             base::UTF8ToUTF16(devices_[0]->client_name())),
-        vector_icons::kCallIcon);
+        ui::ImageModel::FromVectorIcon(controller_->GetVectorIcon(),
+                                       /*color_id=*/-1,
+                                       ui::SimpleMenuModel::kDefaultIconSize));
 #endif
   } else {
     BuildSubMenu();
@@ -92,7 +94,10 @@ void ClickToCallContextMenuObserver::BuildMenu(
     proxy_->AddSubMenuWithStringIdAndIcon(
         IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_MULTIPLE_DEVICES,
         IDS_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_MULTIPLE_DEVICES,
-        sub_menu_model_.get(), vector_icons::kCallIcon);
+        sub_menu_model_.get(),
+        ui::ImageModel::FromVectorIcon(controller_->GetVectorIcon(),
+                                       /*color_id=*/-1,
+                                       ui::SimpleMenuModel::kDefaultIconSize));
 #endif
   }
 }

chrome/browser/sharing/shared_clipboard/shared_clipboard_context_menu_observer.cc

@@ -15,6 +15,7 @@
 #include "chrome/grit/generated_resources.h"
 #include "components/sync_device_info/device_info.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/image_model.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
 
@@ -73,7 +74,9 @@ void SharedClipboardContextMenuObserver::InitMenu(
         l10n_util::GetStringFUTF16(
             IDS_CONTENT_CONTEXT_SHARING_SHARED_CLIPBOARD_SINGLE_DEVICE,
             base::UTF8ToUTF16(devices_[0]->client_name())),
-        controller_->GetVectorIcon());
+        ui::ImageModel::FromVectorIcon(controller_->GetVectorIcon(),
+                                       /*color_id=*/-1,
+                                       ui::SimpleMenuModel::kDefaultIconSize));
 #endif
   } else {
     BuildSubMenu();
@@ -87,7 +90,10 @@ void SharedClipboardContextMenuObserver::InitMenu(
     proxy_->AddSubMenuWithStringIdAndIcon(
         IDC_CONTENT_CONTEXT_SHARING_SHARED_CLIPBOARD_MULTIPLE_DEVICES,
         IDS_CONTENT_CONTEXT_SHARING_SHARED_CLIPBOARD_MULTIPLE_DEVICES,
-        sub_menu_model_.get(), controller_->GetVectorIcon());
+        sub_menu_model_.get(),
+        ui::ImageModel::FromVectorIcon(controller_->GetVectorIcon(),
+                                       /*color_id=*/-1,
+                                       ui::SimpleMenuModel::kDefaultIconSize));
 #endif
   }
 }

components/renderer_context_menu/render_view_context_menu_base.cc

@@ -209,17 +209,8 @@ void RenderViewContextMenuBase::AddMenuItem(int command_id,
 void RenderViewContextMenuBase::AddMenuItemWithIcon(
     int command_id,
     const base::string16& title,
-    const gfx::ImageSkia& image) {
-  menu_model_.AddItemWithIcon(command_id, title,
-                              ui::ImageModel::FromImageSkia(image));
-}
-
-void RenderViewContextMenuBase::AddMenuItemWithIcon(
-    int command_id,
-    const base::string16& title,
-    const gfx::VectorIcon& icon) {
-  menu_model_.AddItemWithIcon(command_id, title,
-                              ui::ImageModel::FromVectorIcon(icon));
+    const ui::ImageModel& icon) {
+  menu_model_.AddItemWithIcon(command_id, title, icon);
 }
 
 void RenderViewContextMenuBase::AddCheckItem(int command_id,
@@ -241,18 +232,9 @@ void RenderViewContextMenuBase::AddSubMenuWithStringIdAndIcon(
     int command_id,
     int message_id,
     ui::MenuModel* model,
-    const gfx::ImageSkia& image) {
-  menu_model_.AddSubMenuWithStringIdAndIcon(
-      command_id, message_id, model, ui::ImageModel::FromImageSkia(image));
-}
-
-void RenderViewContextMenuBase::AddSubMenuWithStringIdAndIcon(
-    int command_id,
-    int message_id,
-    ui::MenuModel* model,
-    const gfx::VectorIcon& icon) {
-  menu_model_.AddSubMenuWithStringIdAndIcon(
-      command_id, message_id, model, ui::ImageModel::FromVectorIcon(icon));
+    const ui::ImageModel& icon) {
+  menu_model_.AddSubMenuWithStringIdAndIcon(command_id, message_id, model,
+                                            icon);
 }
 
 void RenderViewContextMenuBase::UpdateMenuItem(int command_id,
@@ -271,12 +253,12 @@ void RenderViewContextMenuBase::UpdateMenuItem(int command_id,
 }
 
 void RenderViewContextMenuBase::UpdateMenuIcon(int command_id,
-                                               const gfx::Image& image) {
+                                               const ui::ImageModel& icon) {
   int index = menu_model_.GetIndexOfCommandId(command_id);
   if (index == -1)
     return;
 
-  menu_model_.SetIcon(index, ui::ImageModel::FromImage(image));
+  menu_model_.SetIcon(index, icon);
 #if defined(OS_CHROMEOS)
   if (toolkit_delegate_)
     toolkit_delegate_->RebuildMenu();

components/renderer_context_menu/render_view_context_menu_base.h

@@ -97,10 +97,7 @@ class RenderViewContextMenuBase : public ui::SimpleMenuModel::Delegate,
   void AddMenuItem(int command_id, const base::string16& title) override;
   void AddMenuItemWithIcon(int command_id,
                            const base::string16& title,
-                           const gfx::ImageSkia& image) override;
-  void AddMenuItemWithIcon(int command_id,
-                           const base::string16& title,
-                           const gfx::VectorIcon& icon) override;
+                           const ui::ImageModel& icon) override;
   void AddCheckItem(int command_id, const base::string16& title) override;
   void AddSeparator() override;
   void AddSubMenu(int command_id,
@@ -109,16 +106,12 @@ class RenderViewContextMenuBase : public ui::SimpleMenuModel::Delegate,
   void AddSubMenuWithStringIdAndIcon(int command_id,
                                      int message_id,
                                      ui::MenuModel* model,
-                                     const gfx::ImageSkia& image) override;
-  void AddSubMenuWithStringIdAndIcon(int command_id,
-                                     int message_id,
-                                     ui::MenuModel* model,
-                                     const gfx::VectorIcon& icon) override;
+                                     const ui::ImageModel& icon) override;
   void UpdateMenuItem(int command_id,
                       bool enabled,
                       bool hidden,
                       const base::string16& title) override;
-  void UpdateMenuIcon(int command_id, const gfx::Image& image) override;
+  void UpdateMenuIcon(int command_id, const ui::ImageModel& icon) override;
   void RemoveMenuItem(int command_id) override;
   void RemoveAdjacentSeparators() override;
   content::RenderViewHost* GetRenderViewHost() const override;

components/renderer_context_menu/render_view_context_menu_proxy.h

@@ -13,13 +13,8 @@ class RenderViewHost;
 class WebContents;
 }
 
-namespace gfx {
-class Image;
-class ImageSkia;
-struct VectorIcon;
-}
-
 namespace ui {
+class ImageModel;
 class MenuModel;
 }
 
@@ -84,10 +79,7 @@ class RenderViewContextMenuProxy {
   virtual void AddMenuItem(int command_id, const base::string16& title) = 0;
   virtual void AddMenuItemWithIcon(int command_id,
                                    const base::string16& title,
-                                   const gfx::ImageSkia& image) = 0;
-  virtual void AddMenuItemWithIcon(int command_id,
-                                   const base::string16& title,
-                                   const gfx::VectorIcon& image) = 0;
+                                   const ui::ImageModel& icon) = 0;
   virtual void AddCheckItem(int command_id, const base::string16& title) = 0;
   virtual void AddSeparator() = 0;
 
@@ -98,11 +90,7 @@ class RenderViewContextMenuProxy {
   virtual void AddSubMenuWithStringIdAndIcon(int command_id,
                                              int message_id,
                                              ui::MenuModel* model,
-                                             const gfx::ImageSkia& image) = 0;
-  virtual void AddSubMenuWithStringIdAndIcon(int command_id,
-                                             int message_id,
-                                             ui::MenuModel* model,
-                                             const gfx::VectorIcon& image) = 0;
+                                             const ui::ImageModel& icon) = 0;
 
   // Update the status and text of the specified context-menu item.
   virtual void UpdateMenuItem(int command_id,
@@ -111,7 +99,7 @@ class RenderViewContextMenuProxy {
                               const base::string16& title) = 0;
 
   // Update the icon of the specified context-menu item.
-  virtual void UpdateMenuIcon(int command_id, const gfx::Image& image) = 0;
+  virtual void UpdateMenuIcon(int command_id, const ui::ImageModel& icon) = 0;
 
   // Remove the specified context-menu item.
   virtual void RemoveMenuItem(int command_id) = 0;

ui/base/models/simple_menu_model.h

@@ -27,6 +27,9 @@ class ButtonMenuItemModel;
 // The breadth of MenuModel is not exposed through this API.
 class COMPONENT_EXPORT(UI_BASE) SimpleMenuModel : public MenuModel {
  public:
+  // Default icon size to be used for context menus.
+  static constexpr int kDefaultIconSize = 16;
+
   class COMPONENT_EXPORT(UI_BASE) Delegate : public AcceleratorProvider {
    public:
     ~Delegate() override {}