Fix incorrect action icon size in PWA touch mode.

This was causing e.g. the save password icon to not appear or appear
incorrectly in some cases, see attached bug.

New solution is to have the PWA actions container directly implement the
container interface and override the insets logic, which has been
simplified to return the insets and not a whole border object.

Followup:
 - Use the same logic to fix a wonky insets (and inkdrop) size in touch
   mode with the Butter chip

Bug: 1059659
Change-Id: Ia92753e55be5f43b34718cc9901f5273badf3eba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2096117
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Caroline Rising <corising@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748798}

chrome/browser/ui/views/page_action/page_action_icon_controller.cc

@@ -260,6 +260,12 @@ void PageActionIconController::ZoomChangedForActiveTab(bool can_show_bubble) {
     zoom_icon_->ZoomChangedForActiveTab(can_show_bubble);
 }
 
+std::vector<const PageActionIconView*>
+PageActionIconController::GetPageActionIconViewsForTesting() const {
+  return std::vector<const PageActionIconView*>(page_action_icons_.begin(),
+                                                page_action_icons_.end());
+}
+
 void PageActionIconController::OnDefaultZoomLevelChanged() {
   ZoomChangedForActiveTab(false);
 }

chrome/browser/ui/views/page_action/page_action_icon_controller.h

@@ -70,6 +70,9 @@ class PageActionIconController : public zoom::ZoomEventManagerObserver {
   // See comment in browser_window.h for more info.
   void ZoomChangedForActiveTab(bool can_show_bubble);
 
+  std::vector<const PageActionIconView*> GetPageActionIconViewsForTesting()
+      const;
+
  private:
   // ZoomEventManagerObserver:
   // Updates the view for the zoom icon when default zoom levels change.

chrome/browser/ui/views/page_action/page_action_icon_view.cc

@@ -30,10 +30,9 @@ float PageActionIconView::Delegate::GetPageActionInkDropVisibleOpacity() const {
   return GetOmniboxStateOpacity(OmniboxPartState::SELECTED);
 }
 
-std::unique_ptr<views::Border>
-PageActionIconView::Delegate::CreatePageActionIconBorder() const {
-  return views::CreateEmptyBorder(
-      GetLayoutInsets(LOCATION_BAR_ICON_INTERIOR_PADDING));
+gfx::Insets PageActionIconView::Delegate::GetPageActionIconInsets(
+    const PageActionIconView* icon_view) const {
+  return GetLayoutInsets(LOCATION_BAR_ICON_INTERIOR_PADDING);
 }
 
 bool PageActionIconView::Delegate::IsLocationBarUserInputInProgress() const {
@@ -104,8 +103,10 @@ base::string16 PageActionIconView::GetTooltipText(const gfx::Point& p) const {
 void PageActionIconView::ViewHierarchyChanged(
     const views::ViewHierarchyChangedDetails& details) {
   View::ViewHierarchyChanged(details);
-  if (details.is_add && details.child == this && GetNativeTheme())
+  if (details.is_add && details.child == this && GetNativeTheme()) {
     UpdateIconImage();
+    UpdateBorder();
+  }
 }
 
 void PageActionIconView::OnThemeChanged() {
@@ -239,5 +240,7 @@ content::WebContents* PageActionIconView::GetWebContents() const {
 }
 
 void PageActionIconView::UpdateBorder() {
-  SetBorder(delegate_->CreatePageActionIconBorder());
+  const gfx::Insets new_insets = delegate_->GetPageActionIconInsets(this);
+  if (new_insets != GetInsets())
+    SetBorder(views::CreateEmptyBorder(new_insets));
 }

chrome/browser/ui/views/page_action/page_action_icon_view.h

@@ -45,9 +45,9 @@ class PageActionIconView : public IconLabelBubbleView {
 
     virtual content::WebContents* GetWebContentsForPageActionIconView() = 0;
 
-    // Returns the border the icon should use. It depends on what kind of
-    // delegate this icon has.
-    virtual std::unique_ptr<views::Border> CreatePageActionIconBorder() const;
+    // Returns the size of the insets in which the icon should draw its inkdrop.
+    virtual gfx::Insets GetPageActionIconInsets(
+        const PageActionIconView* icon_view) const;
 
     // Delegate should override and return true when the user is editing the
     // location bar contents.

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

@@ -94,12 +94,12 @@ ToolbarAccountIconContainerView::GetWebContentsForPageActionIconView() {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
 
-std::unique_ptr<views::Border>
-ToolbarAccountIconContainerView::CreatePageActionIconBorder() const {
-  // With this border, the icon will have the same ink drop shape as toolbar
-  // buttons.
-  return views::CreateEmptyBorder(ChromeLayoutProvider::Get()->GetInsetsMetric(
-      views::InsetsMetric::INSETS_LABEL_BUTTON));
+gfx::Insets ToolbarAccountIconContainerView::GetPageActionIconInsets(
+    const PageActionIconView* icon_view) const {
+  // Ideally, the icon should have the same ink drop shape as toolbar buttons.
+  // TODO(crbug.com/1060250): fix actual inkdrop shape.
+  return ChromeLayoutProvider::Get()->GetInsetsMetric(
+      views::InsetsMetric::INSETS_LABEL_BUTTON);
 }
 
 void ToolbarAccountIconContainerView::OnThemeChanged() {

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

@@ -39,7 +39,8 @@ class ToolbarAccountIconContainerView : public ToolbarIconContainerView,
   // PageActionIconView::Delegate:
   float GetPageActionInkDropVisibleOpacity() const override;
   content::WebContents* GetWebContentsForPageActionIconView() override;
-  std::unique_ptr<views::Border> CreatePageActionIconBorder() const override;
+  gfx::Insets GetPageActionIconInsets(
+      const PageActionIconView* icon_view) const override;
 
   // views::View:
   void OnThemeChanged() override;

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

@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
+#include "chrome/browser/ui/views/page_action/page_action_icon_controller.h"
 #include "chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -37,6 +38,16 @@ namespace {
 constexpr double kTitlePaddingWidthFraction = 0.1;
 #endif
 
+template <typename T>
+T* GetLastVisible(const std::vector<T*>& views) {
+  T* visible = nullptr;
+  for (auto* view : views) {
+    if (view->GetVisible())
+      visible = view;
+  }
+  return visible;
+}
+
 }  // namespace
 
 class WebAppFrameToolbarBrowserTest : public WebAppFrameToolbarTest {
@@ -77,9 +88,12 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
       web_app_frame_toolbar()->GetRightContainerForTesting();
   EXPECT_EQ(toolbar_right_container->parent(), web_app_frame_toolbar());
 
-  views::View* const page_action_icon_container =
-      web_app_frame_toolbar()->GetPageActionIconContainerForTesting();
-  EXPECT_EQ(page_action_icon_container->parent(), toolbar_right_container);
+  std::vector<const PageActionIconView*> page_actions =
+      web_app_frame_toolbar()
+          ->GetPageActionIconControllerForTesting()
+          ->GetPageActionIconViewsForTesting();
+  for (const PageActionIconView* action : page_actions)
+    EXPECT_EQ(action->parent(), toolbar_right_container);
 
   views::View* const menu_button =
       browser_view()->toolbar_button_provider()->GetAppMenuButton();
@@ -98,7 +112,7 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
 #endif
 
   // Initially the page action icons are not visible.
-  EXPECT_EQ(page_action_icon_container->width(), 0);
+  EXPECT_EQ(GetLastVisible(page_actions), nullptr);
   const int original_menu_button_width = menu_button->width();
   EXPECT_GT(original_menu_button_width, 0);
 
@@ -121,14 +135,14 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
   EXPECT_LT(window_title->width(), original_window_title_width);
 #endif
 
-  EXPECT_GT(page_action_icon_container->width(), 0);
+  EXPECT_NE(GetLastVisible(page_actions), nullptr);
   EXPECT_EQ(menu_button->width(), original_menu_button_width);
 
   // Resize the WebAppFrameToolbarView just enough to clip out the page action
   // icons (and toolbar contents left of them).
   const int original_toolbar_width = web_app_frame_toolbar()->width();
   const int new_toolbar_width = toolbar_right_container->width() -
-                                page_action_icon_container->bounds().right();
+                                GetLastVisible(page_actions)->bounds().right();
   const int new_frame_width =
       frame_view()->width() - original_toolbar_width + new_toolbar_width;
 
@@ -146,7 +160,7 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
 
   // The page action icons should be hidden while the app menu button retains
   // its full width.
-  EXPECT_FALSE(page_action_icon_container->GetVisible());
+  EXPECT_EQ(GetLastVisible(page_actions), nullptr);
   EXPECT_EQ(menu_button->width(), original_menu_button_width);
 }
 

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

@@ -370,15 +370,18 @@ class WebAppFrameToolbarView::ToolbarButtonContainer
       public ContentSettingImageView::Delegate,
       public ImmersiveModeController::Observer,
       public PageActionIconView::Delegate,
+      public PageActionIconContainer,
       public views::WidgetObserver {
  public:
-  ToolbarButtonContainer(views::Widget* widget, BrowserView* browser_view);
+  ToolbarButtonContainer(views::Widget* widget,
+                         BrowserView* browser_view,
+                         ToolbarButtonProvider* toolbar_button_provider);
   ~ToolbarButtonContainer() override;
 
   void UpdateStatusIconsVisibility() {
     if (content_settings_container_)
       content_settings_container_->UpdateContentSettingViewsVisibility();
-    page_action_icon_container_view_->controller()->UpdateAll();
+    page_action_icon_controller_->UpdateAll();
   }
 
   void SetColors(SkColor foreground_color, SkColor background_color) {
@@ -390,8 +393,7 @@ class WebAppFrameToolbarView::ToolbarButtonContainer
       content_settings_container_->SetIconColor(foreground_color_);
     if (extensions_container_)
       extensions_container_->OverrideIconColor(foreground_color_);
-    page_action_icon_container_view_->controller()->SetIconColor(
-        foreground_color_);
+    page_action_icon_controller_->SetIconColor(foreground_color_);
     web_app_menu_button_->SetColor(foreground_color_);
   }
 
@@ -417,8 +419,8 @@ class WebAppFrameToolbarView::ToolbarButtonContainer
     return content_settings_container_;
   }
 
-  PageActionIconContainerView* page_action_icon_container_view() {
-    return page_action_icon_container_view_;
+  PageActionIconController* page_action_icon_controller() {
+    return page_action_icon_controller_.get();
   }
 
   BrowserActionsContainer* browser_actions_container() {
@@ -432,6 +434,26 @@ class WebAppFrameToolbarView::ToolbarButtonContainer
   WebAppMenuButton* web_app_menu_button() { return web_app_menu_button_; }
 
  private:
+  // PageActionIconContainer:
+  void AddPageActionIcon(views::View* icon) override {
+    AddChildViewAt(icon, page_action_insertion_point_++);
+    views::SetHitTestComponent(icon, static_cast<int>(HTCLIENT));
+  }
+
+  // PageActionIconView::Delegate:
+  gfx::Insets GetPageActionIconInsets(
+      const PageActionIconView* icon_view) const override {
+    const int icon_size =
+        icon_view->GetImageView()->GetPreferredSize().height();
+    if (icon_size == 0)
+      return gfx::Insets();
+
+    const int height =
+        toolbar_button_provider_->GetToolbarButtonSize().height();
+    const int inset_size = std::max(0, (height - icon_size) / 2);
+    return gfx::Insets(inset_size);
+  }
+
   // Methods for coordinate the titlebar animation (origin text slide, menu
   // highlight and icon fade in).
   bool ShouldAnimate() const {
@@ -529,14 +551,17 @@ class WebAppFrameToolbarView::ToolbarButtonContainer
 
   // The containing browser view.
   BrowserView* const browser_view_;
+  ToolbarButtonProvider* const toolbar_button_provider_;
 
   SkColor foreground_color_ = gfx::kPlaceholderColor;
   SkColor background_color_ = gfx::kPlaceholderColor;
 
+  std::unique_ptr<PageActionIconController> page_action_icon_controller_;
+  int page_action_insertion_point_ = 0;
+
   // All remaining members are owned by the views hierarchy.
   WebAppOriginText* web_app_origin_text_ = nullptr;
   ContentSettingsContainer* content_settings_container_ = nullptr;
-  PageActionIconContainerView* page_action_icon_container_view_ = nullptr;
   BrowserActionsContainer* browser_actions_container_ = nullptr;
   ExtensionsToolbarContainer* extensions_container_ = nullptr;
   WebAppMenuButton* web_app_menu_button_ = nullptr;
@@ -544,8 +569,12 @@ class WebAppFrameToolbarView::ToolbarButtonContainer
 
 WebAppFrameToolbarView::ToolbarButtonContainer::ToolbarButtonContainer(
     views::Widget* widget,
-    BrowserView* browser_view)
-    : browser_view_(browser_view) {
+    BrowserView* browser_view,
+    ToolbarButtonProvider* toolbar_button_provider)
+    : browser_view_(browser_view),
+      toolbar_button_provider_(toolbar_button_provider),
+      page_action_icon_controller_(
+          std::make_unique<PageActionIconController>()) {
   views::FlexLayout* const layout =
       SetLayoutManager(std::make_unique<views::FlexLayout>());
   layout->SetOrientation(views::LayoutOrientation::kHorizontal)
@@ -578,6 +607,10 @@ WebAppFrameToolbarView::ToolbarButtonContainer::ToolbarButtonContainer(
                                static_cast<int>(HTCLIENT));
   }
 
+  // This is the point where we will be inserting page action icons.
+  page_action_insertion_point_ = int{children().size()};
+
+  // Insert the default page action icons.
   PageActionIconParams params;
   params.types_enabled.push_back(PageActionIconType::kFind);
   params.types_enabled.push_back(PageActionIconType::kManagePasswords);
@@ -596,10 +629,7 @@ WebAppFrameToolbarView::ToolbarButtonContainer::ToolbarButtonContainer(
   params.command_updater = browser_view_->browser()->command_controller();
   params.icon_label_bubble_delegate = this;
   params.page_action_icon_delegate = this;
-  page_action_icon_container_view_ =
-      AddChildView(std::make_unique<PageActionIconContainerView>(params));
-  views::SetHitTestComponent(page_action_icon_container_view_,
-                             static_cast<int>(HTCLIENT));
+  page_action_icon_controller_->Init(params, this);
 
   // Extensions toolbar area with pinned extensions is lower priority than, for
   // example, the menu button or other toolbar buttons, and pinned extensions
@@ -715,7 +745,7 @@ WebAppFrameToolbarView::WebAppFrameToolbarView(views::Widget* widget,
           .WithOrder(3));
 
   right_container_ = AddChildView(
-      std::make_unique<ToolbarButtonContainer>(widget, browser_view));
+      std::make_unique<ToolbarButtonContainer>(widget, browser_view, this));
   right_container_->web_app_menu_button()->SetMinSize(GetToolbarButtonSize());
   right_container_->SetProperty(
       views::kFlexBehaviorKey,
@@ -815,9 +845,7 @@ views::View* WebAppFrameToolbarView::GetDefaultExtensionDialogAnchorView() {
 
 PageActionIconView* WebAppFrameToolbarView::GetPageActionIconView(
     PageActionIconType type) {
-  return right_container_->page_action_icon_container_view()
-      ->controller()
-      ->GetIconView(type);
+  return right_container_->page_action_icon_controller()->GetIconView(type);
 }
 
 AppMenuButton* WebAppFrameToolbarView::GetAppMenuButton() {
@@ -859,9 +887,8 @@ views::View* WebAppFrameToolbarView::GetAnchorView(PageActionIconType type) {
 }
 
 void WebAppFrameToolbarView::ZoomChangedForActiveTab(bool can_show_bubble) {
-  right_container_->page_action_icon_container_view()
-      ->controller()
-      ->ZoomChangedForActiveTab(can_show_bubble);
+  right_container_->page_action_icon_controller()->ZoomChangedForActiveTab(
+      can_show_bubble);
 }
 
 AvatarToolbarButton* WebAppFrameToolbarView::GetAvatarToolbarButton() {
@@ -888,8 +915,9 @@ views::View* WebAppFrameToolbarView::GetRightContainerForTesting() {
   return right_container_;
 }
 
-views::View* WebAppFrameToolbarView::GetPageActionIconContainerForTesting() {
-  return right_container_->page_action_icon_container_view();
+PageActionIconController*
+WebAppFrameToolbarView::GetPageActionIconControllerForTesting() {
+  return right_container_->page_action_icon_controller();
 }
 
 const char* WebAppFrameToolbarView::GetClassName() const {

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

@@ -23,6 +23,7 @@ class Widget;
 
 class BrowserView;
 class ContentSettingImageView;
+class PageActionIconController;
 
 #if defined(OS_MACOSX)
 constexpr int kWebAppMenuMargin = 7;
@@ -87,7 +88,7 @@ class WebAppFrameToolbarView : public views::AccessiblePaneView,
   static void DisableAnimationForTesting();
   views::View* GetLeftContainerForTesting();
   views::View* GetRightContainerForTesting();
-  views::View* GetPageActionIconContainerForTesting();
+  PageActionIconController* GetPageActionIconControllerForTesting();
 
  protected:
   // views::AccessiblePaneView: