Make the chevron menu button responsible for legacy overflow menu logic

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

Index: chrome/browser/ui/views/toolbar/browser_actions_container.cc
diff --git a/chrome/browser/ui/views/toolbar/browser_actions_container.cc b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
index e96c18da5637c1158769fe7d7ee6f38f133993a7..a395e5ff7fdfe4d26aedd7e3de258f30ac556e93 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/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
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -126,12 +97,9 @@ BrowserActionsContainer::BrowserActionsContainer(
       model_(NULL),
       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,12 +124,13 @@ 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);
+      chevron_.reset(new ChevronMenuButton(this));
+      chevron_->set_owned_by_client();

[Peter Kasting, 2014/09/25 01:06:43]

Why do we want to own this ourselves instead of letting the view hierarchy own
it like is typical, and just keep a raw pointer to it?

[Devlin, 2014/09/25 15:56:49]

ChevronMenuButton takes (and uses) a raw pointer to |this|
BrowserActionsContainer, so we need to make sure that |this| outlives the
button.  Admittedly, since this object would own the view as a child, that would
be the case with the current view hierarchy, but I think this way it's a more
explicit (and less fragile if for some reason we move the chevron somewhere). 
For clarity, I think I'd prefer to keep it like this (it certainly doesn't harm
it), but if you'd prefer me to remove it, I'm happy to do so. :)

[Peter Kasting, 2014/09/25 19:06:18]

Hmm.  I see what you mean.

I think I probably prefer the view-hierarchy-owns-the-objects route in the end
just because it's more consistent with how we'd do this elsewhere, but I think
it would be fine to put comments in either or both of these classes at some
appropriate place noting this lifetime requirement.  For example, a comment here
could be:

      // 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_.reset(new ChevronMenuButton(this));

[Devlin, 2014/09/25 20:04:09]

Done.

       chevron_->EnableCanvasFlippingForRTLUI(true);
       chevron_->SetAccessibleName(
           l10n_util::GetStringUTF16(IDS_ACCNAME_EXTENSIONS_CHEVRON));
       chevron_->SetVisible(false);
-      AddChildView(chevron_);
+      AddChildView(chevron_.get());
     }
   }
 }
@@ -171,11 +140,8 @@ BrowserActionsContainer::~BrowserActionsContainer() {
                     observers_,
                     OnBrowserActionsContainerDestroyed());
 
-  if (overflow_menu_)
-    overflow_menu_->set_observer(NULL);
   if (model_)
     model_->RemoveObserver(this);
-  StopShowFolderDropMenuTimer();
   HideActivePopup();
   DeleteBrowserActionViews();
 }
@@ -233,8 +199,6 @@ void BrowserActionsContainer::CreateBrowserActionViews() {
 
 void BrowserActionsContainer::DeleteBrowserActionViews() {
   HideActivePopup();
-  if (overflow_menu_)
-    overflow_menu_->NotifyBrowserActionViewsDeleting();
   STLDeleteElements(&browser_action_views_);
 }
 
@@ -294,8 +258,8 @@ void BrowserActionsContainer::OnBrowserActionViewDragDone() {
 views::MenuButton* BrowserActionsContainer::GetOverflowReferenceView() {
   // With traditional overflow, the reference is the chevron. With the
   // redesign, we use the wrench menu instead.
-  return chevron_ ?
-      chevron_ :
+  return chevron_.get() ?

[Peter Kasting, 2014/09/25 01:06:43]

Nit: Don't use get() when using a scoped_ptr<> as a bool (it's unnecessary)
(multiple places)

[Devlin, 2014/09/25 15:56:49]

For some reason, I thought we were getting rid of this, but apparently not. 
(Un)done.

[Peter Kasting, 2014/09/25 19:06:18]

We're getting rid of implicit conversion of scoped_refptr<> to something (raw
pointer?), maybe that's what you're thinking of.

+      chevron_.get() :
       BrowserView::GetBrowserViewForBrowser(browser_)->toolbar()->app_menu();
 }
 
@@ -382,7 +346,9 @@ 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_.get()) {
     chevron_->SetVisible(true);
     gfx::Size chevron_size(chevron_->GetPreferredSize());
     max_x -=
@@ -392,7 +358,7 @@ void BrowserActionsContainer::Layout() {
         0,
         chevron_size.width(),
         chevron_size.height());
-  } else if (chevron_) {
+  } else if (chevron_.get()) {
     chevron_->SetVisible(false);
   }
 
@@ -447,14 +413,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 +492,6 @@ int BrowserActionsContainer::OnDragUpdated(
 }
 
 void BrowserActionsContainer::OnDragExited() {
-  StopShowFolderDropMenuTimer();
   drop_position_.reset();
   SchedulePaint();
 }
@@ -598,17 +555,7 @@ void BrowserActionsContainer::GetAccessibleState(
 
 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());
-  }
+  DCHECK(false);

[Peter Kasting, 2014/09/25 01:06:43]

Why are we even overriding this method, then?

[Devlin, 2014/09/25 15:56:49]

Whoops!  Missed that one in the cleanup (had it as DCHECK false during testing).
 Thanks :)

 }
 
 void BrowserActionsContainer::WriteDragDataForView(View* sender,
@@ -692,12 +639,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 +760,8 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
            "exists.";
   }
 #endif
-  CloseOverflowMenu();
+  if (chevron_.get())
+    chevron_->CloseMenu();
 
   if (!ShouldDisplayBrowserAction(extension))
     return;
@@ -852,7 +794,7 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
     int model_icon_count = model_->GetVisibleIconCount();
     if (model_icon_count == -1 ||
         (static_cast<size_t>(model_icon_count) < browser_action_views_.size() &&
-         (chevron_ && !chevron_->visible()))) {
+         (chevron_.get() && !chevron_->visible()))) {
       suppress_chevron_ = true;
       Animate(gfx::Tween::LINEAR, GetIconCount());
       return;
@@ -866,7 +808,8 @@ void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
 
 void BrowserActionsContainer::ToolbarExtensionRemoved(
     const Extension* extension) {
-  CloseOverflowMenu();
+  if (chevron_.get())
+    chevron_->CloseMenu();
 
   size_t visible_actions = VisibleBrowserActionsAfterAnimation();
   for (BrowserActionViews::iterator i(browser_action_views_.begin());
@@ -890,7 +833,7 @@ void BrowserActionsContainer::ToolbarExtensionRemoved(
       } else {
         // Either we went from overflow to no-overflow, or we shrunk the no-
         // overflow container by 1.  Either way the size changed, so animate.
-        if (chevron_)
+        if (chevron_.get())
           chevron_->SetVisible(false);
         Animate(gfx::Tween::EASE_OUT, browser_action_views_.size());
       }
@@ -971,7 +914,7 @@ Browser* BrowserActionsContainer::GetBrowser() {
 
 void BrowserActionsContainer::LoadImages() {
   ui::ThemeProvider* tp = GetThemeProvider();
-  if (!tp || !chevron_)
+  if (!tp || !chevron_.get())
     return;
 
   chevron_->SetImage(views::Button::STATE_NORMAL,
@@ -998,46 +941,16 @@ int BrowserActionsContainer::GetPreferredWidth() {
   size_t visible_actions = GetIconCount();
   return IconCountToWidth(
       visible_actions,
-      chevron_ && visible_actions < browser_action_views_.size());
+      chevron_.get() && visible_actions < browser_action_views_.size());
 }
 
 void BrowserActionsContainer::SetChevronVisibility() {
-  if (chevron_) {
+  if (chevron_.get()) {
     chevron_->SetVisible(
         VisibleBrowserActionsAfterAnimation() < browser_action_views_.size());
   }
 }
 
-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)
@@ -1046,7 +959,7 @@ int BrowserActionsContainer::IconCountToWidth(int icons,
     return ToolbarView::kStandardSpacing;
   int icons_size =
       (icons == 0) ? 0 : ((icons * IconWidth(true)) - kItemSpacing);
-  int chevron_size = chevron_ && display_chevron ?
+  int chevron_size = chevron_.get() && display_chevron ?
       (kChevronSpacing + chevron_->GetPreferredSize().width()) : 0;
   // In overflow mode, our padding is to use item spacing on either end (just so
   // we can see the drop indicator). Otherwise we use the standard toolbar
@@ -1066,7 +979,7 @@ size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
   // We reserve space for the padding on either side of the toolbar...
   int available_space = pixels - (ToolbarView::kStandardSpacing * 2);
   // ... and, if the chevron is enabled, the chevron.
-  if (chevron_)
+  if (chevron_.get())
     available_space -= (chevron_->GetPreferredSize().width() + kChevronSpacing);
 
   // Now we add an extra between-item padding value so the space can be divided
@@ -1076,7 +989,7 @@ size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
 }
 
 int BrowserActionsContainer::MinimumNonemptyWidth() const {
-  if (!chevron_)
+  if (!chevron_.get())
     return ToolbarView::kStandardSpacing;
   return (ToolbarView::kStandardSpacing * 2) + kChevronSpacing +
       chevron_->GetPreferredSize().width();