Make the chevron menu button responsible for legacy overflow menu logic

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

 // Copyright 2013 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.
 
 #include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
 
 #include "base/compiler_specific.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ skipping 34 matching lines @@
 #include "ui/views/painter.h"
 #include "ui/views/widget/widget.h"
 
 using extensions::Extension;
 
 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
 
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionsContainer::DropPosition
 
 struct BrowserActionsContainer::DropPosition {
   DropPosition(size_t row, size_t icon_in_row);
 
   // The (0-indexed) row into which the action will be dropped.
   size_t row;
@@ skipping 25 matching lines @@
     BrowserActionsContainer* main_container)
     : initialized_(false),
       profile_(browser->profile()),
       browser_(browser),
       owner_view_(owner_view),
       main_container_(main_container),
       popup_owner_(NULL),
       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());
   if (model_)
     model_->AddObserver(this);
 
   bool overflow_experiment =
       extensions::FeatureSwitch::extension_action_redesign()->IsEnabled();
   DCHECK(!in_overflow_mode() || overflow_experiment);
 
   if (!in_overflow_mode()) {
     extension_keybinding_registry_.reset(new ExtensionKeybindingRegistryViews(
         browser->profile(),
         owner_view->GetFocusManager(),
         extensions::ExtensionKeybindingRegistry::ALL_EXTENSIONS,
         this));
 
     resize_animation_.reset(new gfx::SlideAnimation(this));
     resize_area_ = new views::ResizeArea(this);
     AddChildView(resize_area_);
 
     // '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());
     }
   }
 }
 
 BrowserActionsContainer::~BrowserActionsContainer() {
   FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
                     observers_,
                     OnBrowserActionsContainerDestroyed());
 
-  if (overflow_menu_)
-    overflow_menu_->set_observer(NULL);
   if (model_)
     model_->RemoveObserver(this);
-  StopShowFolderDropMenuTimer();
   HideActivePopup();
   DeleteBrowserActionViews();
 }
 
 void BrowserActionsContainer::Init() {
   LoadImages();
 
   // We wait to set the container width until now so that the chevron images
   // will be loaded.  The width calculation needs to know the chevron size.
   if (model_ && model_->extensions_initialized()) {
@@ skipping 37 matching lines @@
                               action_manager->GetExtensionAction(**i),
                               browser_,
                               this);
     browser_action_views_.push_back(view);
     AddChildView(view);
   }
 }
 
 void BrowserActionsContainer::DeleteBrowserActionViews() {
   HideActivePopup();
-  if (overflow_menu_)
-    overflow_menu_->NotifyBrowserActionViewsDeleting();
   STLDeleteElements(&browser_action_views_);
 }
 
 size_t BrowserActionsContainer::VisibleBrowserActions() const {
   size_t visible_actions = 0;
   for (size_t i = 0; i < browser_action_views_.size(); ++i) {
     if (browser_action_views_[i]->visible())
       ++visible_actions;
   }
   return visible_actions;
@@ skipping 39 matching lines @@
 void BrowserActionsContainer::OnBrowserActionViewDragDone() {
   ToolbarVisibleCountChanged();
   FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
                     observers_,
                     OnBrowserActionDragDone());
 }
 
 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();
 }
 
 void BrowserActionsContainer::SetPopupOwner(BrowserActionView* popup_owner) {
   // We should never be setting a popup owner when one already exists, and
   // never unsetting one when one wasn't set.
   DCHECK((!popup_owner_ && popup_owner) ||
          (popup_owner_ && !popup_owner));
   popup_owner_ = popup_owner;
 }
@@ skipping 66 matching lines @@
     SetVisible(false);
     return;
   }
 
   SetVisible(true);
   if (resize_area_)
     resize_area_->SetBounds(0, 0, kItemSpacing, height());
 
   // 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 -=
         ToolbarView::kStandardSpacing + chevron_size.width() + kChevronSpacing;
     chevron_->SetBounds(
         width() - ToolbarView::kStandardSpacing - chevron_size.width(),
         0,
         chevron_size.width(),
         chevron_size.height());
-  } else if (chevron_) {
+  } else if (chevron_.get()) {
     chevron_->SetVisible(false);
   }
 
   // Now draw the icons for the browser actions in the available space.
   int icon_width = IconWidth(false);
   if (in_overflow_mode()) {
     for (size_t i = 0;
          i < main_container_->VisibleBrowserActionsAfterAnimation(); ++i) {
       // Ensure that any browser actions shown in the main view are hidden in
       // the overflow view.
@@ skipping 34 matching lines @@
 bool BrowserActionsContainer::AreDropTypesRequired() {
   return BrowserActionDragData::AreDropTypesRequired();
 }
 
 bool BrowserActionsContainer::CanDrop(const OSExchangeData& data) {
   return BrowserActionDragData::CanDrop(data, profile_);
 }
 
 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
   // an empty overflow/main container), then 0, 0 for row, column is correct.
   if (VisibleBrowserActions() != 0) {
     // Figure out where to display the indicator. This is a complex calculation:
 
     // First, we subtract out the padding to the left of the icon area, which is
     // ToolbarView::kStandardSpacing. If we're right-to-left, we also mirror the
     // event.x() so that our calculations are consistent with left-to-right.
@@ skipping 59 matching lines @@
       !(drop_position_->row == row_index &&
         drop_position_->icon_in_row == before_icon_in_row)) {
     drop_position_.reset(new DropPosition(row_index, before_icon_in_row));
     SchedulePaint();
   }
 
   return ui::DragDropTypes::DRAG_MOVE;
 }
 
 void BrowserActionsContainer::OnDragExited() {
-  StopShowFolderDropMenuTimer();
   drop_position_.reset();
   SchedulePaint();
 }
 
 int BrowserActionsContainer::OnPerformDrop(
     const ui::DropTargetEvent& event) {
   BrowserActionDragData data;
   if (!data.Read(event.data()))
     return ui::DragDropTypes::DRAG_NONE;
 
@@ skipping 43 matching lines @@
 }
 
 void BrowserActionsContainer::GetAccessibleState(
     ui::AXViewState* state) {
   state->role = ui::AX_ROLE_GROUP;
   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());
-  }
+  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,
                                                    const gfx::Point& press_pt,
                                                    OSExchangeData* data) {
   DCHECK(data);
 
   for (size_t i = 0; i < browser_action_views_.size(); ++i) {
     BrowserActionView* view = browser_action_views_[i];
     if (view == sender) {
@@ skipping 63 matching lines @@
   resize_amount_ = 0;
   suppress_chevron_ = false;
   SetChevronVisibility();
   OnBrowserActionVisibilityChanged();
 
   FOR_EACH_OBSERVER(BrowserActionsContainerObserver,
                     observers_,
                     OnBrowserActionsContainerAnimationEnded());
 }
 
-void BrowserActionsContainer::NotifyMenuDeleted(
-    BrowserActionOverflowMenuController* controller) {
-  DCHECK_EQ(overflow_menu_, controller);
-  overflow_menu_ = NULL;
-}
-
 content::WebContents* BrowserActionsContainer::GetCurrentWebContents() {
   return browser_->tab_strip_model()->GetActiveWebContents();
 }
 
 extensions::ActiveTabPermissionGranter*
     BrowserActionsContainer::GetActiveTabPermissionGranter() {
   content::WebContents* web_contents =
       browser_->tab_strip_model()->GetActiveWebContents();
   if (!web_contents)
     return NULL;
@@ skipping 101 matching lines @@
 
 void BrowserActionsContainer::ToolbarExtensionAdded(const Extension* extension,
                                                     int index) {
 #if defined(DEBUG)
   for (size_t i = 0; i < browser_action_views_.size(); ++i) {
     DCHECK(browser_action_views_[i]->extension() != extension) <<
            "Asked to add a browser action view for an extension that already "
            "exists.";
   }
 #endif
-  CloseOverflowMenu();
+  if (chevron_.get())
+    chevron_->CloseMenu();
 
   if (!ShouldDisplayBrowserAction(extension))
     return;
 
   // Add the new browser action to the vector and the view hierarchy.
   if (profile_->IsOffTheRecord())
     index = model_->OriginalIndexToIncognito(index);
   BrowserActionView* view =
       new BrowserActionView(extension,
                             extensions::ExtensionActionManager::Get(profile_)->
@@ skipping 12 matching lines @@
           IsBeingUpgraded(extension)) {
     // We need to resize if either:
     // - The container is set to display all icons (visible count = -1), or
     // - The container will need to expand to include the chevron. This can
     //   happen when the container is set to display <n> icons, where <n> is
     //   the number of icons before the new icon. With the new icon, the chevron
     //   will need to be displayed.
     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;
     }
   }
 
   // Otherwise, we don't have to resize, so just redraw the (possibly modified)
   // visible icon set.
   OnBrowserActionVisibilityChanged();
 }
 
 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());
        i != browser_action_views_.end(); ++i) {
     if ((*i)->extension() == extension) {
       delete *i;
       browser_action_views_.erase(i);
 
       // If the extension is being upgraded we don't want the bar to shrink
       // because the icon is just going to get re-added to the same location.
       if (extensions::ExtensionSystem::Get(profile_)->runtime_data()->
               IsBeingUpgraded(extension))
         return;
 
       if (browser_action_views_.size() > visible_actions) {
         // If we have more icons than we can show, then we must not be changing
         // the container size (since we either removed an icon from the main
         // area and one from the overflow list will have shifted in, or we
         // removed an entry directly from the overflow list).
         OnBrowserActionVisibilityChanged();
       } 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());
       }
       return;  // We have found the action to remove, bail out.
     }
   }
 }
 
 void BrowserActionsContainer::ToolbarExtensionMoved(const Extension* extension,
                                                     int index) {
@@ skipping 60 matching lines @@
   CreateBrowserActionViews();
   Animate(gfx::Tween::LINEAR, GetIconCount());
 }
 
 Browser* BrowserActionsContainer::GetBrowser() {
   return browser_;
 }
 
 void BrowserActionsContainer::LoadImages() {
   ui::ThemeProvider* tp = GetThemeProvider();
-  if (!tp || !chevron_)
+  if (!tp || !chevron_.get())
     return;
 
   chevron_->SetImage(views::Button::STATE_NORMAL,
                      *tp->GetImageSkiaNamed(IDR_BROWSER_ACTIONS_OVERFLOW));
 
   const int kImages[] = IMAGE_GRID(IDR_DEVELOPER_MODE_HIGHLIGHT);
   highlight_painter_.reset(views::Painter::CreateImageGridPainter(kImages));
 }
 
 void BrowserActionsContainer::OnBrowserActionVisibilityChanged() {
   SetVisible(!browser_action_views_.empty());
   if (owner_view_) {
     owner_view_->Layout();
     owner_view_->SchedulePaint();
   } else {
     // In overflow mode, we don't have an owner view, but we still have to
     // update ourselves.
     Layout();
     SchedulePaint();
   }
 }
 
 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)
     icons = browser_action_views_.size();
   if ((icons == 0) && !display_chevron)
     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
   // spacing.
   // Note: These are actually the same thing, but, on the offchance one
   // changes, let's get it right.
   int padding =
       2 * (in_overflow_mode() ? kItemSpacing : ToolbarView::kStandardSpacing);
   return icons_size + chevron_size + padding;
 }
 
 size_t BrowserActionsContainer::WidthToIconCount(int pixels) const {
   // Check for widths large enough to show the entire icon set.
   if (pixels >= IconCountToWidth(-1, false))
     return browser_action_views_.size();
 
   // 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
   // evenly by (size of icon with padding).
   return static_cast<size_t>(
       std::max(0, available_space + kItemSpacing) / IconWidth(true));
 }
 
 int BrowserActionsContainer::MinimumNonemptyWidth() const {
-  if (!chevron_)
+  if (!chevron_.get())
     return ToolbarView::kStandardSpacing;
   return (ToolbarView::kStandardSpacing * 2) + kChevronSpacing +
       chevron_->GetPreferredSize().width();
 }
 
 void BrowserActionsContainer::Animate(gfx::Tween::Type tween_type,
                                       size_t num_visible_icons) {
   int target_size = IconCountToWidth(num_visible_icons,
       num_visible_icons < browser_action_views_.size());
   if (resize_animation_ && !disable_animations_during_testing_) {
@@ skipping 46 matching lines @@
   if (initialized_ && profile_->IsOffTheRecord()) {
     main_displayed = in_overflow_mode() ?
         main_container_->VisibleBrowserActionsAfterAnimation() :
         VisibleBrowserActionsAfterAnimation();
   }
 
   // The overflow displays any (displayable) icons not shown by the main bar.
   return in_overflow_mode() ?
       displayable_icon_count - main_displayed : main_displayed;
 }