Move nested pin button from ExtensionsMenuButton into its parent view.

Bug: 985382

Change-Id: I67f4df0176f802901ef08860f312f02d0bd18ce7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1774312Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#693021}

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

@@ -4,41 +4,18 @@
 
 #include "chrome/browser/ui/views/extensions/extensions_menu_button.h"
 
-#include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
-#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
 #include "chrome/browser/ui/views/extensions/extensions_menu_item_view.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_view.h"
-#include "chrome/grit/generated_resources.h"
-#include "ui/base/l10n/l10n_util.h"
-#include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/controls/button/button.h"
-#include "ui/views/controls/button/image_button.h"
-#include "ui/views/controls/button/image_button_factory.h"
-#include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/button/menu_button_controller.h"
 #include "ui/views/layout/box_layout.h"
-#include "ui/views/layout/layout_provider.h"
-#include "ui/views/vector_icons.h"
-#include "ui/views/view_class_properties.h"
 
 const char ExtensionsMenuButton::kClassName[] = "ExtensionsMenuButton";
 
-namespace {
-
-constexpr int EXTENSION_PINNING = 14;
-
-void SetSecondaryButtonHighlightPath(views::Button* button) {
-  auto highlight_path = std::make_unique<SkPath>();
-  highlight_path->addOval(gfx::RectToSkRect(gfx::Rect(button->size())));
-  button->SetProperty(views::kHighlightPathKey, highlight_path.release());
-}
-
-}  // namespace
-
 ExtensionsMenuButton::ExtensionsMenuButton(
     Browser* browser,
     ExtensionsMenuItemView* parent,
@@ -52,12 +29,7 @@ ExtensionsMenuButton::ExtensionsMenuButton(
                   true),
       browser_(browser),
       parent_(parent),
-      controller_(controller),
-      model_(ToolbarActionsModel::Get(browser_->profile())) {
-  // Set so the extension button receives enter/exit on children to retain hover
-  // status when hovering child views.
-  set_notify_enter_exit_on_child(true);
-  ConfigureSecondaryView();
+      controller_(controller) {
   set_auto_compute_tooltip(false);
   controller_->SetDelegate(this);
   UpdateState();
@@ -65,53 +37,12 @@ ExtensionsMenuButton::ExtensionsMenuButton(
 
 ExtensionsMenuButton::~ExtensionsMenuButton() = default;
 
-void ExtensionsMenuButton::UpdatePinButton() {
-  pin_button_->SetTooltipText(l10n_util::GetStringUTF16(
-      IsPinned() ? IDS_EXTENSIONS_MENU_UNPIN_BUTTON_TOOLTIP
-                 : IDS_EXTENSIONS_MENU_PIN_BUTTON_TOOLTIP));
-  SkColor unpinned_icon_color =
-      ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors()
-          ? gfx::kGoogleGrey500
-          : gfx::kChromeIconGrey;
-  SkColor icon_color = IsPinned()
-                           ? GetNativeTheme()->GetSystemColor(
-                                 ui::NativeTheme::kColorId_ProminentButtonColor)
-                           : unpinned_icon_color;
-  views::SetImageFromVectorIcon(
-      pin_button_, IsPinned() ? views::kUnpinIcon : views::kPinIcon,
-      ExtensionsMenuItemView::kSecondaryIconSizeDp, icon_color);
-  pin_button_->SetVisible(IsPinned() || IsMouseHovered() || IsMenuRunning());
-}
-
-void ExtensionsMenuButton::OnMouseEntered(const ui::MouseEvent& event) {
-  UpdatePinButton();
-  // The layout manager does not get notified of visibility changes and the pin
-  // buttons has not be laid out before if it was invisible.
-  pin_button_->InvalidateLayout();
-  views::Button::OnMouseEntered(event);
-}
-
-void ExtensionsMenuButton::OnMouseExited(const ui::MouseEvent& event) {
-  UpdatePinButton();
-  views::Button::OnMouseExited(event);
-}
-
-void ExtensionsMenuButton::OnBoundsChanged(const gfx::Rect& previous_bounds) {
-  UpdatePinButton();
-  HoverButton::OnBoundsChanged(previous_bounds);
-}
-
 const char* ExtensionsMenuButton::GetClassName() const {
   return kClassName;
 }
 
 void ExtensionsMenuButton::ButtonPressed(Button* sender,
                                          const ui::Event& event) {
-  if (sender->GetID() == EXTENSION_PINNING) {
-    model_->SetActionVisibility(controller_->GetId(), !IsPinned());
-    return;
-  }
-  DCHECK_EQ(this, sender);
   controller_->ExecuteAction(true);
 }
 
@@ -150,30 +81,3 @@ void ExtensionsMenuButton::UpdateState() {
 bool ExtensionsMenuButton::IsMenuRunning() const {
   return parent_->IsContextMenuRunning();
 }
-
-void ExtensionsMenuButton::ConfigureSecondaryView() {
-  views::View* container = secondary_view();
-  DCHECK(container->children().empty());
-  container->SetLayoutManager(std::make_unique<views::BoxLayout>(
-      views::BoxLayout::Orientation::kHorizontal));
-
-  const SkColor icon_color =
-      ui::NativeTheme::GetInstanceForNativeUi()->GetSystemColor(
-          ui::NativeTheme::kColorId_DefaultIconColor);
-
-  auto pin_button = views::CreateVectorImageButton(this);
-  pin_button->SetID(EXTENSION_PINNING);
-  pin_button->set_ink_drop_base_color(icon_color);
-  pin_button->SizeToPreferredSize();
-
-  pin_button_ = pin_button.get();
-  SetSecondaryButtonHighlightPath(pin_button_);
-  container->AddChildView(std::move(pin_button));
-}
-
-bool ExtensionsMenuButton::IsPinned() {
-  // |model_| can be null in unit tests.
-  if (!model_)
-    return false;
-  return model_->IsActionPinned(controller_->GetId());
-}

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

@@ -18,7 +18,6 @@ class ExtensionsMenuItemView;
 
 namespace views {
 class Button;
-class ImageButton;
 }  // namespace views
 
 class ExtensionsMenuButton : public HoverButton,
@@ -37,11 +36,6 @@ class ExtensionsMenuButton : public HoverButton,
   static const char kClassName[];
 
  private:
-  // views::Button:
-  void OnMouseEntered(const ui::MouseEvent& event) override;
-  void OnMouseExited(const ui::MouseEvent& event) override;
-  void OnBoundsChanged(const gfx::Rect& previous_bounds) override;
-
   // views::ButtonListener:
   const char* GetClassName() const override;
   void ButtonPressed(Button* sender, const ui::Event& event) override;
@@ -54,11 +48,6 @@ class ExtensionsMenuButton : public HoverButton,
   void UpdateState() override;
   bool IsMenuRunning() const override;
 
-  // Configures the secondary (right-hand-side) view of this HoverButton.
-  void ConfigureSecondaryView();
-
-  bool IsPinned();
-
   Browser* const browser_;
 
   // The container containing this view.
@@ -67,10 +56,6 @@ class ExtensionsMenuButton : public HoverButton,
   // Responsible for executing the extension's actions.
   ToolbarActionViewController* const controller_;
 
-  ToolbarActionsModel* const model_;
-
-  views::ImageButton* pin_button_ = nullptr;
-
   DISALLOW_COPY_AND_ASSIGN(ExtensionsMenuButton);
 };
 

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

@@ -7,20 +7,23 @@
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/toolbar/toolbar_action_view_controller.h"
+#include "chrome/browser/ui/toolbar/toolbar_actions_model.h"
 #include "chrome/browser/ui/views/extensions/extension_context_menu_controller.h"
 #include "chrome/browser/ui/views/extensions/extensions_menu_button.h"
 #include "chrome/grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/animation/ink_drop_host_view.h"
-#include "ui/views/layout/fill_layout.h"
+#include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/layout/flex_layout.h"
 #include "ui/views/layout/flex_layout_types.h"
-#include "ui/views/layout/grid_layout.h"
+#include "ui/views/vector_icons.h"
 #include "ui/views/view_class_properties.h"
 
 namespace {
 constexpr int EXTENSION_CONTEXT_MENU = 13;
+constexpr int EXTENSION_PINNING = 14;
 }  // namespace
 
 ExtensionsMenuItemView::ExtensionsMenuItemView(
@@ -28,7 +31,12 @@ ExtensionsMenuItemView::ExtensionsMenuItemView(
     std::unique_ptr<ToolbarActionViewController> controller)
     : primary_action_button_(
           new ExtensionsMenuButton(browser, this, controller.get())),
-      controller_(std::move(controller)) {
+      controller_(std::move(controller)),
+      model_(ToolbarActionsModel::Get(browser->profile())) {
+  // Set so the extension button receives enter/exit on children to retain hover
+  // status when hovering child views.
+  set_notify_enter_exit_on_child(true);
+
   context_menu_controller_ = std::make_unique<ExtensionContextMenuController>(
       nullptr, controller_.get());
 
@@ -48,6 +56,12 @@ ExtensionsMenuItemView::ExtensionsMenuItemView(
       ui::NativeTheme::GetInstanceForNativeUi()->GetSystemColor(
           ui::NativeTheme::kColorId_DefaultIconColor);
 
+  auto pin_button = views::CreateVectorImageButton(this);
+  pin_button->SetID(EXTENSION_PINNING);
+  pin_button->set_ink_drop_base_color(icon_color);
+  pin_button_ = pin_button.get();
+  AddChildView(std::move(pin_button));
+
   // TODO(pbos): There's complicated configuration code in place since menus
   // can't be triggered from ImageButtons. When MenuRunner::RunMenuAt accepts
   // views::Buttons, turn this into a views::ImageButton and use
@@ -79,6 +93,15 @@ ExtensionsMenuItemView::ExtensionsMenuItemView(
 
 ExtensionsMenuItemView::~ExtensionsMenuItemView() = default;
 
+void ExtensionsMenuItemView::ButtonPressed(views::Button* sender,
+                                           const ui::Event& event) {
+  if (sender->GetID() == EXTENSION_PINNING) {
+    model_->SetActionVisibility(controller_->GetId(), !IsPinned());
+    return;
+  }
+  NOTREACHED();
+}
+
 void ExtensionsMenuItemView::OnMenuButtonClicked(views::Button* source,
                                                  const gfx::Point& point,
                                                  const ui::Event* event) {
@@ -90,13 +113,43 @@ void ExtensionsMenuItemView::OnMenuButtonClicked(views::Button* source,
 }
 
 void ExtensionsMenuItemView::UpdatePinButton() {
-  primary_action_button_->UpdatePinButton();
+  pin_button_->SetTooltipText(l10n_util::GetStringUTF16(
+      IsPinned() ? IDS_EXTENSIONS_MENU_UNPIN_BUTTON_TOOLTIP
+                 : IDS_EXTENSIONS_MENU_PIN_BUTTON_TOOLTIP));
+  SkColor unpinned_icon_color =
+      ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors()
+          ? gfx::kGoogleGrey500
+          : gfx::kChromeIconGrey;
+  SkColor icon_color = IsPinned()
+                           ? GetNativeTheme()->GetSystemColor(
+                                 ui::NativeTheme::kColorId_ProminentButtonColor)
+                           : unpinned_icon_color;
+  views::SetImageFromVectorIcon(
+      pin_button_, IsPinned() ? views::kUnpinIcon : views::kPinIcon,
+      kSecondaryIconSizeDp, icon_color);
+  pin_button_->SetVisible(IsPinned() || IsMouseHovered() ||
+                          IsContextMenuRunning());
+}
+
+void ExtensionsMenuItemView::OnMouseEntered(const ui::MouseEvent& event) {
+  UpdatePinButton();
+}
+
+void ExtensionsMenuItemView::OnMouseExited(const ui::MouseEvent& event) {
+  UpdatePinButton();
 }
 
 bool ExtensionsMenuItemView::IsContextMenuRunning() {
   return context_menu_controller_->IsMenuRunning();
 }
 
+bool ExtensionsMenuItemView::IsPinned() {
+  // |model_| can be null in unit tests.
+  if (!model_)
+    return false;
+  return model_->IsActionPinned(controller_->GetId());
+}
+
 ExtensionsMenuButton*
 ExtensionsMenuItemView::primary_action_button_for_testing() {
   return primary_action_button_;

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

@@ -7,16 +7,18 @@
 
 #include <memory>
 
+#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/menu_button_listener.h"
-#include "ui/views/layout/flex_layout.h"
 #include "ui/views/view.h"
 
 class Browser;
 class ExtensionContextMenuController;
 class ExtensionsMenuButton;
 class ToolbarActionViewController;
+class ToolbarActionsModel;
 
 namespace views {
+class ImageButton;
 class MenuButton;
 }  // namespace views
 
@@ -25,6 +27,7 @@ class MenuButton;
 // a button to pin the extension to the toolbar and a button for accessing the
 // associated context menu.
 class ExtensionsMenuItemView : public views::View,
+                               public views::ButtonListener,
                                public views::MenuButtonListener {
  public:
   static constexpr int kSecondaryIconSizeDp = 16;
@@ -34,6 +37,9 @@ class ExtensionsMenuItemView : public views::View,
       std::unique_ptr<ToolbarActionViewController> controller);
   ~ExtensionsMenuItemView() override;
 
+  // views::ButtonListener:
+  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
+
   // views::MenuButtonListener:
   void OnMenuButtonClicked(views::Button* source,
                            const gfx::Point& point,
@@ -43,15 +49,25 @@ class ExtensionsMenuItemView : public views::View,
 
   bool IsContextMenuRunning();
 
+  bool IsPinned();
+
   ExtensionsMenuButton* primary_action_button_for_testing();
 
  private:
+  // views::Button:
+  void OnMouseEntered(const ui::MouseEvent& event) override;
+  void OnMouseExited(const ui::MouseEvent& event) override;
+
   ExtensionsMenuButton* const primary_action_button_;
 
   std::unique_ptr<ToolbarActionViewController> controller_;
 
   views::MenuButton* context_menu_button_ = nullptr;
 
+  ToolbarActionsModel* const model_;
+
+  views::ImageButton* pin_button_ = nullptr;
+
   // This controller is responsible for showing the context menu for an
   // extension.
   std::unique_ptr<ExtensionContextMenuController> context_menu_controller_;