Allow the user to drag an extension to the overflow menu, even when its empty

We need to show an empty bar for the user to drag extension actions into for
overflow when there are none currently overflowed. Obviously, this empty bar
should only be displayed when the user is dragging an extension action.

BUG=414386

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

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

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

@@ -268,8 +268,15 @@ void BrowserActionsContainer::ExecuteExtensionCommand(
 void BrowserActionsContainer::NotifyActionMovedToOverflow() {
   // When an action is moved to overflow, we shrink the size of the container
   // by 1.
-  if (!profile_->IsOffTheRecord())
-    model_->SetVisibleIconCount(model_->GetVisibleIconCount() - 1);
+  if (!profile_->IsOffTheRecord()) {
+    int icon_count = model_->GetVisibleIconCount();
+    // Since this happens when an icon moves from the main bar to overflow, we
+    // can't possibly have had no visible icons on the main bar.
+    DCHECK_NE(0, icon_count);
+    if (icon_count == -1)
+      icon_count = browser_action_views_.size();
+    model_->SetVisibleIconCount(icon_count - 1);
+  }
   Animate(gfx::Tween::EASE_OUT,
           VisibleBrowserActionsAfterAnimation() - 1);
 }
@@ -322,24 +329,23 @@ void BrowserActionsContainer::RemoveObserver(
 }
 
 gfx::Size BrowserActionsContainer::GetPreferredSize() const {
-  // Note: We can't use GetIconCount() for the main bar, since we may also
-  // have to include items that are in the chevron's overflow.
-  size_t icon_count =
-      in_overflow_mode() ? GetIconCount() : browser_action_views_.size();
-
-  // If there are no actions to show, or we are in overflow mode and the main
-  // container is already showing them all, then no further work is required.
-  if (icon_count == 0)
-    return gfx::Size();
-
   if (in_overflow_mode()) {
-    // When in overflow, y is multiline, so the pixel count is IconHeight()
-    // times the number of rows needed.
+    int icon_count = GetIconCount();
+    // In overflow, we always have a preferred size of a full row (even if we
+    // don't use it), and always of at least one row. The parent may decide to
+    // show us even when empty, e.g. as a drag target for dragging in icons from
+    // the main container.
+    int row_count =
+        ((std::max(0, icon_count - 1)) / icons_per_overflow_menu_row_) + 1;
     return gfx::Size(
         IconCountToWidth(icons_per_overflow_menu_row_, false),
-        (((icon_count - 1) / icons_per_overflow_menu_row_) + 1) * IconHeight());
+        row_count * IconHeight());
   }
 
+  // If there are no actions to show, then don't show the container at all.
+  if (browser_action_views_.empty())
+    return gfx::Size();
+
   // We calculate the size of the view by taking the current width and
   // subtracting resize_amount_ (the latter represents how far the user is
   // resizing the view or, if animating the snapping, how far to animate it).
@@ -448,24 +454,25 @@ int BrowserActionsContainer::OnDragUpdated(
   }
   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 figure out how much space is to the left of the icon area, so we
-  // can calculate the true offset into the icon area. The easiest way to do
-  // this is to just find where the first icon starts.
-  int width_before_icons =
-      browser_action_views_[GetFirstVisibleIconIndex()]->x();
-
-  // If we're right-to-left, we flip the mirror the event.x() so that our
-  // calculations are consistent with left-to-right.
+    // 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.
     int offset_into_icon_area =
-      GetMirroredXInView(event.x()) - width_before_icons;
+        GetMirroredXInView(event.x()) - ToolbarView::kStandardSpacing;
 
     // Next, figure out what row we're on. This only matters for overflow mode,
     // but the calculation is the same for both.
-  size_t row_index = event.y() / IconHeight();
+    row_index = event.y() / IconHeight();
 
-  // Sanity check - we should never be on a different row in the main container.
+    // Sanity check - we should never be on a different row in the main
+    // container.
     DCHECK(in_overflow_mode() || row_index == 0);
 
     // Next, we determine which icon to place the indicator in front of. We want
@@ -489,7 +496,8 @@ int BrowserActionsContainer::OnDragUpdated(
     // plus one spacing unit, then it becomes very easy to calculate how many
     // divider points we've passed, because they're the multiples of "one icon
     // plus padding".
-  int before_icon_unclamped = (offset_into_icon_area + (IconWidth(false) / 2) +
+    int before_icon_unclamped =
+        (offset_into_icon_area + (IconWidth(false) / 2) +
         kItemSpacing) / IconWidth(true);
 
     // We need to figure out how many icons are visible on the relevant row.
@@ -506,12 +514,13 @@ int BrowserActionsContainer::OnDragUpdated(
                   icons_per_overflow_menu_row_;
     }
 
-  // Because the user can drag outside the container bounds, we need to clamp to
-  // the valid range.  Note that the maximum allowable value is (num icons), not
-  // (num icons - 1), because we represent the indicator being past the last
-  // icon as being "before the (last + 1) icon".
-  size_t before_icon_in_row =
+    // Because the user can drag outside the container bounds, we need to clamp
+    // to the valid range. Note that the maximum allowable value is (num icons),
+    // not (num icons - 1), because we represent the indicator being past the
+    // last icon as being "before the (last + 1) icon".
+    before_icon_in_row =
         std::min(std::max(before_icon_unclamped, 0), visible_icons_on_row);
+  }
 
   if (!drop_position_.get() ||
       !(drop_position_->row == row_index &&
@@ -543,7 +552,7 @@ int BrowserActionsContainer::OnPerformDrop(
   size_t i = drop_position_->row * icons_per_overflow_menu_row_ +
              drop_position_->icon_in_row;
   if (in_overflow_mode())
-    i += GetFirstVisibleIconIndex();
+    i += main_container_->VisibleBrowserActionsAfterAnimation();
   // |i| now points to the item to the right of the drop indicator*, which is
   // correct when dragging an icon to the left. When dragging to the right,
   // however, we want the icon being dragged to get the index of the item to
@@ -702,11 +711,6 @@ extensions::ActiveTabPermissionGranter*
       active_tab_permission_granter();
 }
 
-size_t BrowserActionsContainer::GetFirstVisibleIconIndex() const {
-  return in_overflow_mode() ?
-      main_container_->VisibleBrowserActionsAfterAnimation() : 0;
-}
-
 ExtensionPopup* BrowserActionsContainer::TestGetPopup() {
   return popup_owner_ ? popup_owner_->view_controller()->popup() : NULL;
 }
@@ -731,7 +735,7 @@ void BrowserActionsContainer::OnPaint(gfx::Canvas* canvas) {
 
     // Convert back to a pixel offset into the container.  First find the X
     // coordinate of the drop icon.
-    int drop_icon_x = browser_action_views_[GetFirstVisibleIconIndex()]->x() +
+    int drop_icon_x = ToolbarView::kStandardSpacing +
         (drop_position_->icon_in_row * IconWidth(true));
     // Next, find the space before the drop icon. This will either be
     // kItemSpacing or ToolbarView::kStandardSpacing, depending on whether this

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

@@ -349,9 +349,6 @@ class BrowserActionsContainer
   // for incognito.
   bool ShouldDisplayBrowserAction(const extensions::Extension* extension) const;
 
-  // Return the index of the first visible icon.
-  size_t GetFirstVisibleIconIndex() const;
-
   // Returns the number of icons that this container should draw. This differs
   // from the model's GetVisibleIconCount if this container is for the overflow.
   size_t GetIconCount() const;

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

@@ -44,6 +44,9 @@ ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
       main);
   container_->Init();
   AddChildView(container_);
+  // We Layout() the container here so that we know the number of actions
+  // that will be visible in ShouldShow().
+  container_->Layout();
 
   // If we were opened for a drop command, we have to wait for the drop to
   // finish so we can close the wrench menu.
@@ -56,6 +59,11 @@ ExtensionToolbarMenuView::ExtensionToolbarMenuView(Browser* browser,
 ExtensionToolbarMenuView::~ExtensionToolbarMenuView() {
 }
 
+bool ExtensionToolbarMenuView::ShouldShow() {
+  return wrench_menu_->for_drop() ||
+      container_->VisibleBrowserActionsAfterAnimation();
+}
+
 gfx::Size ExtensionToolbarMenuView::GetPreferredSize() const {
   return container_->GetPreferredSize();
 }

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

@@ -25,6 +25,11 @@ class ExtensionToolbarMenuView : public views::View,
   ExtensionToolbarMenuView(Browser* browser, WrenchMenu* wrench_menu);
   virtual ~ExtensionToolbarMenuView();
 
+  // Returns whether the wrench menu should show this view. This is true when
+  // either |container_| has icons to display or the menu was opened for a drag-
+  // and-drop operation.
+  bool ShouldShow();
+
   // views::View:
   virtual gfx::Size GetPreferredSize() const OVERRIDE;
   virtual int GetHeightForWidth(int width) const OVERRIDE;

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

@@ -1137,7 +1137,7 @@ void WrenchMenu::PopulateMenu(MenuItemView* parent,
       case IDC_EXTENSIONS_OVERFLOW_MENU: {
         scoped_ptr<ExtensionToolbarMenuView> extension_toolbar(
             new ExtensionToolbarMenuView(browser_, this));
-        if (extension_toolbar->GetPreferredSize().height() > 0)
+        if (extension_toolbar->ShouldShow())
           item->AddChildView(extension_toolbar.release());
         else
           item->SetVisible(false);