Fix extension icon size in PWA.

Extension icons were the wrong size *and* inkdrops were rendering at the
wrong size. This CL creates a common point for determining the
appropriate size for both toolbar icons and extension icons, and by
simplifying how inkdrop size and insets calculations happen.

Bug: 1006162, 1051393, 1055239
Change-Id: I6f5719f9904a21288f3caab0cb0da6a37b9a77f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2042263
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Caroline Rising <corising@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747766}

chrome/browser/ui/views/extensions/extensions_toolbar_button.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/extensions/extensions_toolbar_button.h"
 
 #include "chrome/browser/themes/theme_properties.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/views/extensions/extensions_menu_view.h"
 #include "chrome/browser/ui/views/extensions/extensions_toolbar_container.h"
 #include "chrome/grit/generated_resources.h"
@@ -37,14 +38,44 @@ ExtensionsToolbarButton::ExtensionsToolbarButton(
 }
 
 void ExtensionsToolbarButton::UpdateIcon() {
-  const int icon_size = ui::MaterialDesignController::touch_ui()
-                            ? kDefaultTouchableIconSize
-                            : kDefaultIconSize;
   SetImage(views::Button::STATE_NORMAL,
-           gfx::CreateVectorIcon(vector_icons::kExtensionIcon, icon_size,
+           gfx::CreateVectorIcon(vector_icons::kExtensionIcon, GetIconSize(),
                                  extensions_container_->GetIconColor()));
 }
 
+gfx::Size ExtensionsToolbarButton::CalculatePreferredSize() const {
+  return extensions_container_->GetToolbarActionSize();
+}
+
+void ExtensionsToolbarButton::OnBoundsChanged(
+    const gfx::Rect& previous_bounds) {
+  // Because this button is in a container and doesn't necessarily take up the
+  // whole height of the toolbar, the standard insets calculation does not
+  // apply. Instead calculate the insets as the difference between the icon
+  // size and the preferred button size.
+
+  const gfx::Size current_size = size();
+  if (current_size.IsEmpty())
+    return;
+  const int icon_size = GetIconSize();
+  gfx::Insets new_insets;
+  if (icon_size < current_size.width()) {
+    const int diff = current_size.width() - icon_size;
+    new_insets.set_left(diff / 2);
+    new_insets.set_right((diff + 1) / 2);
+  }
+  if (icon_size < current_size.height()) {
+    const int diff = current_size.height() - icon_size;
+    new_insets.set_top(diff / 2);
+    new_insets.set_bottom((diff + 1) / 2);
+  }
+  SetLayoutInsets(new_insets);
+}
+
+const char* ExtensionsToolbarButton::GetClassName() const {
+  return "ExtensionsToolbarButton";
+}
+
 void ExtensionsToolbarButton::ButtonPressed(views::Button* sender,
                                             const ui::Event& event) {
   if (ExtensionsMenuView::IsShowing()) {
@@ -53,3 +84,9 @@ void ExtensionsToolbarButton::ButtonPressed(views::Button* sender,
   }
   ExtensionsMenuView::ShowBubble(this, browser_, extensions_container_);
 }
+
+int ExtensionsToolbarButton::GetIconSize() const {
+  return ui::MaterialDesignController::touch_ui() && !browser_->app_controller()
+             ? kDefaultTouchableIconSize
+             : kDefaultIconSize;
+}

chrome/browser/ui/views/extensions/extensions_toolbar_button.h

@@ -25,9 +25,16 @@ class ExtensionsToolbarButton : public ToolbarButton,
   void UpdateIcon();
 
  private:
+  // ToolbarButton:
+  gfx::Size CalculatePreferredSize() const override;
+  void OnBoundsChanged(const gfx::Rect& previous_bounds) override;
+  const char* GetClassName() const override;
+
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
+  int GetIconSize() const;
+
   Browser* const browser_;
   views::MenuButtonController* menu_button_controller_;
   ExtensionsToolbarContainer* const extensions_container_;

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -14,7 +14,9 @@
 #include "chrome/browser/ui/views/extensions/browser_action_drag_data.h"
 #include "chrome/browser/ui/views/extensions/extensions_menu_view.h"
 #include "chrome/browser/ui/views/extensions/extensions_toolbar_button.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
+#include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.h"
 #include "ui/views/layout/animating_layout_manager.h"
 #include "ui/views/layout/flex_layout.h"
 #include "ui/views/layout/flex_layout_types.h"
@@ -451,9 +453,12 @@ views::LabelButton* ExtensionsToolbarContainer::GetOverflowReferenceView()
 }
 
 gfx::Size ExtensionsToolbarContainer::GetToolbarActionSize() {
-  gfx::Rect rect(gfx::Size(28, 28));
-  rect.Inset(-GetLayoutInsets(TOOLBAR_ACTION_VIEW));
-  return rect.size();
+  constexpr gfx::Size kDefaultSize(28, 28);
+  BrowserView* const browser_view =
+      BrowserView::GetBrowserViewForBrowser(browser_);
+  return browser_view
+             ? browser_view->toolbar_button_provider()->GetToolbarButtonSize()
+             : kDefaultSize;
 }
 
 void ExtensionsToolbarContainer::WriteDragDataForView(

chrome/browser/ui/views/frame/toolbar_button_provider.h

@@ -17,6 +17,7 @@ class ToolbarButton;
 
 namespace gfx {
 class Rect;
+class Size;
 }
 
 namespace views {
@@ -35,6 +36,9 @@ class ToolbarButtonProvider {
   // Gets the ExtensionsToolbarContainer.
   virtual ExtensionsToolbarContainer* GetExtensionsToolbarContainer() = 0;
 
+  // Get the default size for toolbar buttons.
+  virtual gfx::Size GetToolbarButtonSize() const = 0;
+
   // Gets the default view to use as an anchor for extension dialogs if the
   // ToolbarActionView is not visible or available.
   virtual views::View* GetDefaultExtensionDialogAnchorView() = 0;

chrome/browser/ui/views/toolbar/toolbar_button.cc

@@ -266,6 +266,8 @@ bool ToolbarButton::IsMenuShowing() const {
 }
 
 void ToolbarButton::SetLayoutInsets(const gfx::Insets& insets) {
+  if (layout_insets_ == insets)
+    return;
   layout_insets_ = insets;
   UpdateColorsAndInsets();
 }

chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.cc

@@ -54,7 +54,7 @@ gfx::Insets GetToolbarInkDropInsets(const views::View* host_view) {
   const int inkdrop_dimensions = GetLayoutConstant(LOCATION_BAR_HEIGHT);
   gfx::Insets inkdrop_insets =
       margin_insets +
-      gfx::Insets((host_size.height() - inkdrop_dimensions) / 2);
+      gfx::Insets(std::max(0, (host_size.height() - inkdrop_dimensions) / 2));
 
   return inkdrop_insets;
 }

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -817,6 +817,11 @@ ExtensionsToolbarContainer* ToolbarView::GetExtensionsToolbarContainer() {
   return extensions_container_;
 }
 
+gfx::Size ToolbarView::GetToolbarButtonSize() const {
+  const int size = GetLayoutConstant(LayoutConstant::TOOLBAR_BUTTON_HEIGHT);
+  return gfx::Size(size, size);
+}
+
 views::View* ToolbarView::GetDefaultExtensionDialogAnchorView() {
   if (extensions_container_)
     return extensions_container_->extensions_button();

chrome/browser/ui/views/toolbar/toolbar_view.h

@@ -227,6 +227,7 @@ class ToolbarView : public views::AccessiblePaneView,
   // ToolbarButtonProvider:
   BrowserActionsContainer* GetBrowserActionsContainer() override;
   ExtensionsToolbarContainer* GetExtensionsToolbarContainer() override;
+  gfx::Size GetToolbarButtonSize() const override;
   views::View* GetDefaultExtensionDialogAnchorView() override;
   PageActionIconView* GetPageActionIconView(PageActionIconType type) override;
   AppMenuButton* GetAppMenuButton() override;

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.cc

@@ -137,14 +137,8 @@ int WebAppFrameRightMargin() {
 // Insets are kept small to avoid increasing web app frame toolbar height.
 void SetInsetsForWebAppToolbarButton(ToolbarButton* toolbar_button,
                                      bool is_browser_focus_mode) {
-  if (!is_browser_focus_mode) {
-    // We set the kInternalPaddingKey property first, as SetLayoutInsets caches
-    // the resulting total insets.
-    constexpr gfx::Insets kInkDropInsets(2);
-    toolbar_button->SetProperty(views::kInternalPaddingKey, kInkDropInsets);
-
+  if (!is_browser_focus_mode)
     toolbar_button->SetLayoutInsets(gfx::Insets(2));
-  }
 }
 
 const gfx::VectorIcon& GetBackImage(bool touch_ui) {
@@ -722,6 +716,7 @@ WebAppFrameToolbarView::WebAppFrameToolbarView(views::Widget* widget,
 
   right_container_ = AddChildView(
       std::make_unique<ToolbarButtonContainer>(widget, browser_view));
+  right_container_->web_app_menu_button()->SetMinSize(GetToolbarButtonSize());
   right_container_->SetProperty(
       views::kFlexBehaviorKey,
       views::FlexSpecification(right_container_->GetFlexRule()).WithOrder(1));
@@ -804,6 +799,14 @@ WebAppFrameToolbarView::GetExtensionsToolbarContainer() {
   return right_container_->extensions_container();
 }
 
+gfx::Size WebAppFrameToolbarView::GetToolbarButtonSize() const {
+  constexpr int kFocusModeButtonSize = 34;
+  int size = browser_view_->browser()->is_focus_mode()
+                 ? kFocusModeButtonSize
+                 : GetLayoutConstant(WEB_APP_MENU_BUTTON_SIZE);
+  return gfx::Size(size, size);
+}
+
 views::View* WebAppFrameToolbarView::GetDefaultExtensionDialogAnchorView() {
   if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu))
     return right_container_->extensions_container()->extensions_button();

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.h

@@ -71,6 +71,7 @@ class WebAppFrameToolbarView : public views::AccessiblePaneView,
   // ToolbarButtonProvider:
   BrowserActionsContainer* GetBrowserActionsContainer() override;
   ExtensionsToolbarContainer* GetExtensionsToolbarContainer() override;
+  gfx::Size GetToolbarButtonSize() const override;
   views::View* GetDefaultExtensionDialogAnchorView() override;
   PageActionIconView* GetPageActionIconView(PageActionIconType type) override;
   AppMenuButton* GetAppMenuButton() override;

chrome/browser/ui/views/web_apps/web_app_menu_button.cc

@@ -42,11 +42,6 @@ WebAppMenuButton::WebAppMenuButton(BrowserView* browser_view)
   SetTooltipText(
       l10n_util::GetStringFUTF16(IDS_WEB_APP_MENU_BUTTON_TOOLTIP, app_name));
 
-  constexpr int focus_mode_app_menu_button_size = 34;
-  bool is_focus_mode = browser_view->browser()->is_focus_mode();
-  int size = is_focus_mode ? focus_mode_app_menu_button_size
-                           : GetLayoutConstant(WEB_APP_MENU_BUTTON_SIZE);
-  SetMinSize(gfx::Size(size, size));
   SetHorizontalAlignment(gfx::ALIGN_CENTER);
 }