Make the chevron menu button responsible for legacy overflow menu logic

Move the ChevronMenuButton class into its own file, and move the logic to show
the legacy (drop-down) extension action overflow menu from the
BrowserActionsContainer into there.

This is mainly because:
1. We'll eventually be getting rid of this legacy behavior, and this way, the
   removal is all concentrated in one area (ChevronMenuButton).
2. BrowserActionsContainer is already doing a ton (since it has both new
   overflow and normal logic). It doesn't need this, too.

As a bonus, refactored the lifetime of the overflow menu to be more
deterministic, and got rid of the back-and-forth between it and the
BrowserActionsContainer.

And all with a negative line count! Woohoo!

BUG=417085

Review URL: https://codereview.chromium.org/597783002

Cr-Commit-Position: refs/heads/master@{#297240}

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

-// Copyright (c) 2012 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 CHROME_BROWSER_UI_VIEWS_EXTENSIONS_BROWSER_ACTION_OVERFLOW_MENU_CONTROLLER_H_
-#define CHROME_BROWSER_UI_VIEWS_EXTENSIONS_BROWSER_ACTION_OVERFLOW_MENU_CONTROLLER_H_
-
-#include <set>
-#include <vector>
-
-#include "base/compiler_specific.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/memory/scoped_vector.h"
-#include "base/sequenced_task_runner_helpers.h"
-#include "ui/views/controls/menu/menu_delegate.h"
-
-class Browser;
-class BrowserActionsContainer;
-class BrowserActionView;
-class IconUpdater;
-
-namespace views {
-class MenuRunner;
-class Widget;
-}
-
-// This class handles the overflow menu for browser actions (showing the menu,
-// drag and drop, etc). This class manages its own lifetime.
-class BrowserActionOverflowMenuController : public views::MenuDelegate {
- public:
-  // The observer is notified prior to the menu being deleted.
-  class Observer {
-   public:
-    virtual void NotifyMenuDeleted(
-        BrowserActionOverflowMenuController* controller) = 0;
-  };
-
-  BrowserActionOverflowMenuController(
-      BrowserActionsContainer* owner,
-      Browser* browser,
-      views::MenuButton* menu_button,
-      const std::vector<BrowserActionView*>& views,
-      int start_index,
-      bool for_drop);
-
-  void set_observer(Observer* observer) { observer_ = observer; }
-
-  // Shows the overflow menu.
-  bool RunMenu(views::Widget* widget);
-
-  // Closes the overflow menu (and its context menu if open as well).
-  void CancelMenu();
-
-  // Notify the menu that the associated BrowserActionViews have been deleted.
-  void NotifyBrowserActionViewsDeleting();
-
-  // Overridden from views::MenuDelegate:
-  virtual bool IsCommandEnabled(int id) const OVERRIDE;
-  virtual void ExecuteCommand(int id) OVERRIDE;
-  virtual bool ShowContextMenu(views::MenuItemView* source,
-                               int id,
-                               const gfx::Point& p,
-                               ui::MenuSourceType source_type) OVERRIDE;
-  virtual void DropMenuClosed(views::MenuItemView* menu) OVERRIDE;
-  // These drag functions offer support for dragging icons into the overflow
-  // menu.
-  virtual bool GetDropFormats(
-      views::MenuItemView* menu,
-      int* formats,
-      std::set<ui::OSExchangeData::CustomFormat>* custom_formats) OVERRIDE;
-  virtual bool AreDropTypesRequired(views::MenuItemView* menu) OVERRIDE;
-  virtual bool CanDrop(views::MenuItemView* menu,
-                       const ui::OSExchangeData& data) OVERRIDE;
-  virtual int GetDropOperation(views::MenuItemView* item,
-                               const ui::DropTargetEvent& event,
-                               DropPosition* position) OVERRIDE;
-  virtual int OnPerformDrop(views::MenuItemView* menu,
-                            DropPosition position,
-                            const ui::DropTargetEvent& event) OVERRIDE;
-  // These three drag functions offer support for dragging icons out of the
-  // overflow menu.
-  virtual bool CanDrag(views::MenuItemView* menu) OVERRIDE;
-  virtual void WriteDragData(views::MenuItemView* sender,
-                             ui::OSExchangeData* data) OVERRIDE;
-  virtual int GetDragOperations(views::MenuItemView* sender) OVERRIDE;
-
- private:
-  // This class manages its own lifetime.
-  virtual ~BrowserActionOverflowMenuController();
-
-  // Returns the offset into |views_| for the given |id|.
-  size_t IndexForId(int id) const;
-
-  // A pointer to the browser action container that owns the overflow menu.
-  BrowserActionsContainer* owner_;
-
-  Browser* browser_;
-
-  // The observer, may be null.
-  Observer* observer_;
-
-  // A pointer to the overflow menu button that we are showing the menu for.
-  views::MenuButton* menu_button_;
-
-  // The overflow menu for the menu button. Owned by |menu_runner_|.
-  views::MenuItemView* menu_;
-
-  // Resposible for running the menu.
-  scoped_ptr<views::MenuRunner> menu_runner_;
-
-  // The views vector of all the browser actions the container knows about. We
-  // won't show all items, just the one starting at |start_index| and above.
-  // Owned by |owner_|.
-  const std::vector<BrowserActionView*>& views_;
-
-  // The index into the BrowserActionView vector, indicating where to start
-  // picking browser actions to draw.
-  int start_index_;
-
-  // Whether this controller is being used for drop.
-  bool for_drop_;
-
-  // The vector keeps all icon updaters associated with menu item views in the
-  // controller. The icon updater will update the menu item view's icon when
-  // the browser action view's icon has been updated.
-  ScopedVector<IconUpdater> icon_updaters_;
-
-  friend class base::DeleteHelper<BrowserActionOverflowMenuController>;
-
-  DISALLOW_COPY_AND_ASSIGN(BrowserActionOverflowMenuController);
-};
-
-#endif  // CHROME_BROWSER_UI_VIEWS_EXTENSIONS_BROWSER_ACTION_OVERFLOW_MENU_CONTROLLER_H_

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

@@ -52,35 +52,6 @@ namespace {
 // Horizontal spacing before the chevron (if visible).
 const int kChevronSpacing = ToolbarView::kStandardSpacing - 2;
 
-// A version of MenuButton with almost empty insets to fit properly on the
-// toolbar.
-class ChevronMenuButton : public views::MenuButton {
- public:
-  ChevronMenuButton(views::ButtonListener* listener,
-                    const base::string16& text,
-                    views::MenuButtonListener* menu_button_listener,
-                    bool show_menu_marker)
-      : views::MenuButton(listener,
-                          text,
-                          menu_button_listener,
-                          show_menu_marker) {
-  }
-
-  virtual ~ChevronMenuButton() {}
-
-  virtual scoped_ptr<views::LabelButtonBorder> CreateDefaultBorder() const
-      OVERRIDE {
-    // The chevron resource was designed to not have any insets.
-    scoped_ptr<views::LabelButtonBorder> border =
-        views::MenuButton::CreateDefaultBorder();
-    border->set_insets(gfx::Insets());
-    return border.Pass();
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ChevronMenuButton);
-};
-
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -127,11 +98,9 @@ BrowserActionsContainer::BrowserActionsContainer(
       container_width_(0),
       resize_area_(NULL),
       chevron_(NULL),
-      overflow_menu_(NULL),
       suppress_chevron_(false),
       resize_amount_(0),
-      animation_target_size_(0),
-      show_menu_task_factory_(this) {
+      animation_target_size_(0) {
   set_id(VIEW_ID_BROWSER_ACTION_TOOLBAR);
 
   model_ = extensions::ExtensionToolbarModel::Get(browser->profile());
@@ -156,7 +125,10 @@ BrowserActionsContainer::BrowserActionsContainer(
     // 'Main' mode doesn't need a chevron overflow when overflow is shown inside
     // the Chrome menu.
     if (!overflow_experiment) {
-      chevron_ = new ChevronMenuButton(NULL, base::string16(), this, false);
+      // Since the ChevronMenuButton holds a raw pointer to us, we need to
+      // ensure it doesn't outlive us.  Having it owned by the view hierarchy as
+      // a child will suffice.
+      chevron_ = new ChevronMenuButton(this);
       chevron_->EnableCanvasFlippingForRTLUI(true);
       chevron_->SetAccessibleName(
           l10n_util::GetStringUTF16(IDS_ACCNAME_EXTENSIONS_CHEVRON));
@@ -171,11 +143,8 @@ BrowserActionsContainer::~BrowserActionsContainer() {
                     observers_,
                     OnBrowserActionsContainerDestroyed());
 
-  if (overflow_menu_)
-    overflow_menu_->set_observer(NULL);
   if (model_)
     model_->RemoveObserver(this);
-  StopShowFolderDropMenuTimer();
   HideActivePopup();
   DeleteBrowserActionViews();
 }
@@ -233,8 +202,6 @@ void BrowserActionsContainer::CreateBrowserActionViews() {
 
 void BrowserActionsContainer::DeleteBrowserActionViews() {
   HideActivePopup();
-  if (overflow_menu_)
-    overflow_menu_->NotifyBrowserActionViewsDeleting();
   STLDeleteElements(&browser_action_views_);
 }
 
@@ -382,7 +349,7 @@ void BrowserActionsContainer::Layout() {
 
   // If the icons don't all fit, show the chevron (unless suppressed).
   int max_x = GetPreferredSize().width();
-  if ((IconCountToWidth(-1, false) > max_x) && !suppress_chevron_ && chevron_) {
+  if (IconCountToWidth(-1, false) > max_x && !suppress_chevron_ && chevron_) {
     chevron_->SetVisible(true);
     gfx::Size chevron_size(chevron_->GetPreferredSize());
     max_x -=
@@ -447,14 +414,6 @@ bool BrowserActionsContainer::CanDrop(const OSExchangeData& data) {
 
 int BrowserActionsContainer::OnDragUpdated(
     const ui::DropTargetEvent& event) {
-  // First check if we are above the chevron (overflow) menu.
-  if (chevron_ && GetEventHandlerForPoint(event.location()) == chevron_) {
-    if (!show_menu_task_factory_.HasWeakPtrs() && !overflow_menu_)
-      StartShowFolderDropMenuTimer();
-    return ui::DragDropTypes::DRAG_MOVE;
-  }
-  StopShowFolderDropMenuTimer();
-
   size_t row_index = 0;
   size_t before_icon_in_row = 0;
   // If there are no visible browser actions (such as when dragging an icon to
@@ -534,7 +493,6 @@ int BrowserActionsContainer::OnDragUpdated(
 }
 
 void BrowserActionsContainer::OnDragExited() {
-  StopShowFolderDropMenuTimer();
   drop_position_.reset();
   SchedulePaint();
 }
@@ -596,21 +554,6 @@ void BrowserActionsContainer::GetAccessibleState(
   state->name = l10n_util::GetStringUTF16(IDS_ACCNAME_EXTENSIONS);
 }
 
-void BrowserActionsContainer::OnMenuButtonClicked(views::View* source,
-                                                  const gfx::Point& point) {
-  if (source == chevron_) {
-    overflow_menu_ =
-        new BrowserActionOverflowMenuController(this,
-                                                browser_,
-                                                chevron_,
-                                                browser_action_views_,
-                                                VisibleBrowserActions(),
-                                                false);
-    overflow_menu_->set_observer(this);
-    overflow_menu_->RunMenu(GetWidget());
-  }
-}
-
 void BrowserActionsContainer::WriteDragDataForView(View* sender,
                                                    const gfx::Point& press_pt,
                                                    OSExchangeData* data) {
@@ -692,12 +635,6 @@ void BrowserActionsContainer::AnimationEnded(const gfx::Animation* animation) {
                     OnBrowserActionsContainerAnimationEnded());
 }
 
-void BrowserActionsContainer::NotifyMenuDeleted(
-    BrowserActionOverflowMenuController* controller) {
-  DCHECK_EQ(overflow_menu_, controller);
-  overflow_menu_ = NULL;
-}
-
 content::WebContents* BrowserActionsContainer::GetCurrentWebContents() {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
@@ -819,7 +756,8 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
            "exists.";
   }
 #endif
-  CloseOverflowMenu();
+  if (chevron_)
+    chevron_->CloseMenu();
 
   if (!ShouldDisplayBrowserAction(extension))
     return;
@@ -866,7 +804,8 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
 
 void BrowserActionsContainer::ToolbarExtensionRemoved(
     const Extension* extension) {
-  CloseOverflowMenu();
+  if (chevron_)
+    chevron_->CloseMenu();
 
   size_t visible_actions = VisibleBrowserActionsAfterAnimation();
   for (BrowserActionViews::iterator i(browser_action_views_.begin());
@@ -1008,36 +947,6 @@ void BrowserActionsContainer::SetChevronVisibility() {
   }
 }
 
-void BrowserActionsContainer::CloseOverflowMenu() {
-  if (overflow_menu_)
-    overflow_menu_->CancelMenu();
-}
-
-void BrowserActionsContainer::StopShowFolderDropMenuTimer() {
-  show_menu_task_factory_.InvalidateWeakPtrs();
-}
-
-void BrowserActionsContainer::StartShowFolderDropMenuTimer() {
-  base::MessageLoop::current()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&BrowserActionsContainer::ShowDropFolder,
-                 show_menu_task_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMilliseconds(views::GetMenuShowDelay()));
-}
-
-void BrowserActionsContainer::ShowDropFolder() {
-  DCHECK(!overflow_menu_);
-  overflow_menu_ =
-      new BrowserActionOverflowMenuController(this,
-                                              browser_,
-                                              chevron_,
-                                              browser_action_views_,
-                                              VisibleBrowserActions(),
-                                              true);
-  overflow_menu_->set_observer(this);
-  overflow_menu_->RunMenu(GetWidget());
-}
-
 int BrowserActionsContainer::IconCountToWidth(int icons,
                                               bool display_chevron) const {
   if (icons < 0)

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

@@ -8,9 +8,9 @@
 #include "base/observer_list.h"
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 #include "chrome/browser/extensions/extension_toolbar_model.h"
-#include "chrome/browser/ui/views/extensions/browser_action_overflow_menu_controller.h"
 #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h"
 #include "chrome/browser/ui/views/toolbar/browser_action_view.h"
+#include "chrome/browser/ui/views/toolbar/chevron_menu_button.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/animation/tween.h"
 #include "ui/views/controls/button/menu_button_listener.h"
@@ -123,11 +123,9 @@ class ResizeArea;
 ////////////////////////////////////////////////////////////////////////////////
 class BrowserActionsContainer
     : public views::View,
-      public views::MenuButtonListener,
       public views::ResizeAreaDelegate,
       public gfx::AnimationDelegate,
       public extensions::ExtensionToolbarModel::Observer,
-      public BrowserActionOverflowMenuController::Observer,
       public BrowserActionView::Delegate,
       public extensions::ExtensionKeybindingRegistry::Delegate {
  public:
@@ -218,10 +216,6 @@ class BrowserActionsContainer
   virtual int OnPerformDrop(const ui::DropTargetEvent& event) OVERRIDE;
   virtual void GetAccessibleState(ui::AXViewState* state) OVERRIDE;
 
-  // Overridden from views::MenuButtonListener:
-  virtual void OnMenuButtonClicked(views::View* source,
-                                   const gfx::Point& point) OVERRIDE;
-
   // Overridden from views::DragController:
   virtual void WriteDragDataForView(View* sender,
                                     const gfx::Point& press_pt,
@@ -239,10 +233,6 @@ class BrowserActionsContainer
   virtual void AnimationProgressed(const gfx::Animation* animation) OVERRIDE;
   virtual void AnimationEnded(const gfx::Animation* animation) OVERRIDE;
 
-  // Overridden from BrowserActionOverflowMenuController::Observer:
-  virtual void NotifyMenuDeleted(
-      BrowserActionOverflowMenuController* controller) OVERRIDE;
-
   // Overridden from BrowserActionView::Delegate:
   virtual content::WebContents* GetCurrentWebContents() OVERRIDE;
   virtual bool ShownInsideMenu() const OVERRIDE;
@@ -317,18 +307,6 @@ class BrowserActionsContainer
   // are displayed.
   void SetChevronVisibility();
 
-  // Closes the overflow menu if open.
-  void CloseOverflowMenu();
-
-  // Cancels the timer for showing the drop down menu.
-  void StopShowFolderDropMenuTimer();
-
-  // Show the drop down folder after a slight delay.
-  void StartShowFolderDropMenuTimer();
-
-  // Show the overflow menu.
-  void ShowDropFolder();
-
   // Given a number of |icons| and whether to |display_chevron|, returns the
   // amount of pixels needed to draw the entire container.  For convenience,
   // callers can set |icons| to -1 to mean "all icons".
@@ -398,15 +376,11 @@ class BrowserActionsContainer
 
   // The chevron for accessing the overflow items. Can be NULL when in overflow
   // mode or if the toolbar is permanently suppressing the chevron menu.
-  views::MenuButton* chevron_;
+  ChevronMenuButton* chevron_;
 
   // The painter used when we are highlighting a subset of extensions.
   scoped_ptr<views::Painter> highlight_painter_;
 
-  // The menu to show for the overflow button (chevron). This class manages its
-  // own lifetime so that it can stay alive during drag and drop operations.
-  BrowserActionOverflowMenuController* overflow_menu_;
-
   // The animation that happens when the container snaps to place.
   scoped_ptr<gfx::SlideAnimation> resize_animation_;
 
@@ -435,9 +409,6 @@ class BrowserActionsContainer
   // icons in the application menu).
   static int icons_per_overflow_menu_row_;
 
-  // Handles delayed showing of the overflow menu when hovering.
-  base::WeakPtrFactory<BrowserActionsContainer> show_menu_task_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(BrowserActionsContainer);
 };
 

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

+// Copyright 2014 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 CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHEVRON_MENU_BUTTON_H_
+#define CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHEVRON_MENU_BUTTON_H_
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "ui/views/controls/button/menu_button.h"
+#include "ui/views/controls/button/menu_button_listener.h"
+
+class BrowserActionsContainer;
+
+// The MenuButton for the chevron in the extension toolbar, which is also
+// responsible for showing the legacy (drop-down) overflow menu.
+class ChevronMenuButton : public views::MenuButton,
+                          public views::MenuButtonListener {
+ public:
+  explicit ChevronMenuButton(
+      BrowserActionsContainer* browser_actions_container);
+  virtual ~ChevronMenuButton();
+
+  // Closes the overflow menu (and any context menu), if it is open.
+  void CloseMenu();
+
+ private:
+  class MenuController;
+
+  // views::MenuButton:
+  virtual scoped_ptr<views::LabelButtonBorder> CreateDefaultBorder() const
+      OVERRIDE;
+  virtual bool GetDropFormats(int* formats,
+      std::set<ui::OSExchangeData::CustomFormat>* custom_formats) OVERRIDE;
+  virtual bool AreDropTypesRequired() OVERRIDE;
+  virtual bool CanDrop(const ui::OSExchangeData& data) OVERRIDE;
+  virtual void OnDragEntered(const ui::DropTargetEvent& event) OVERRIDE;
+  virtual int OnDragUpdated(const ui::DropTargetEvent& event) OVERRIDE;
+  virtual void OnDragExited() OVERRIDE;
+  virtual int OnPerformDrop(const ui::DropTargetEvent& event) OVERRIDE;
+
+  // views::MenuButtonListener:
+  virtual void OnMenuButtonClicked(View* source, const gfx::Point& point)
+      OVERRIDE;
+
+  // Shows the overflow menu.
+  void ShowOverflowMenu(bool for_drop);
+
+  // Called by the overflow menu when all the work is done.
+  void MenuDone();
+
+  // The owning BrowserActionsContainer.
+  BrowserActionsContainer* browser_actions_container_;
+
+  // The overflow menu controller.
+  scoped_ptr<MenuController> menu_controller_;
+
+  base::WeakPtrFactory<ChevronMenuButton> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChevronMenuButton);
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHEVRON_MENU_BUTTON_H_

chrome/chrome_browser_ui.gypi

@@ -1973,8 +1973,6 @@
       'browser/ui/views/extensions/bookmark_app_bubble_view.h',
       'browser/ui/views/extensions/browser_action_drag_data.cc',
       'browser/ui/views/extensions/browser_action_drag_data.h',
-      'browser/ui/views/extensions/browser_action_overflow_menu_controller.cc',
-      'browser/ui/views/extensions/browser_action_overflow_menu_controller.h',
       'browser/ui/views/extensions/bundle_installed_bubble.cc',
       'browser/ui/views/extensions/extension_action_view_controller.cc',
       'browser/ui/views/extensions/extension_action_view_controller.h',
@@ -2232,6 +2230,8 @@
       'browser/ui/views/toolbar/browser_action_test_util_views.cc',
       'browser/ui/views/toolbar/browser_action_view.cc',
       'browser/ui/views/toolbar/browser_action_view.h',
+      'browser/ui/views/toolbar/chevron_menu_button.cc',
+      'browser/ui/views/toolbar/chevron_menu_button.h',
       'browser/ui/views/toolbar/extension_toolbar_menu_view.cc',
       'browser/ui/views/toolbar/extension_toolbar_menu_view.h',
       'browser/ui/views/toolbar/home_button.cc',