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

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 250 matching lines @@
   // Global commands are handled by the ExtensionCommandsGlobalRegistry
   // instance.
   DCHECK(!command.global());
   extension_keybinding_registry_->ExecuteCommand(extension->id(),
                                                  command.accelerator());
 }
 
 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();
+    if (icon_count == -1)
+      icon_count = browser_action_views_.size();
+    model_->SetVisibleIconCount(icon_count - 1);

[Peter Kasting, 2014/09/16 21:40:54]

What if |icon_count| is 0?  Do we really want to call with -1?

[Devlin, 2014/09/16 21:58:29]

Interestingly, VisibleIconCount() *cannot* be 0 here, as this is saying that an
icon was moved from the main bar to overflow.  As such, there must have been at
least one visible icon.  Added a DCHECK and a comment.

+  }
   Animate(gfx::Tween::EASE_OUT,
           VisibleBrowserActionsAfterAnimation() - 1);
 }
 
 bool BrowserActionsContainer::ShownInsideMenu() const {
   return in_overflow_mode();
 }
 
 void BrowserActionsContainer::OnBrowserActionViewDragDone() {
   ToolbarVisibleCountChanged();
@@ skipping 32 matching lines @@
     BrowserActionsContainerObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void BrowserActionsContainer::RemoveObserver(
     BrowserActionsContainerObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 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. If there's nothing to
+    // display, the container won't be shown.

[Peter Kasting, 2014/09/16 21:40:54]

Nit: Isn't this comment incorrect, in that sometimes we show the container when
there's nothing to display (the point of this change)?

[Devlin, 2014/09/16 21:58:29]

Fair - I was counting a "drop area" as "something to be shown".  But
disambiguated it.

+    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());
   }
+  DCHECK(!in_overflow_mode());

[Peter Kasting, 2014/09/16 21:40:54]

Nit: I'd remove this DCHECK as it's obviously true given the line just above.

[Devlin, 2014/09/16 21:58:29]

Done.

+
+  // If there are no actions to show, then don't show the container at all.
+  if (browser_action_views_.size() == 0)

[Peter Kasting, 2014/09/16 21:40:54]

Nit: empty()?

[Devlin, 2014/09/16 21:58:29]

Done.

+    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).
   // But we also clamp it to a minimum size and the maximum size, so that the
   // container can never shrink too far or take up more space than it needs.
   // In other words: MinimumNonemptyWidth() < width() - resize < ClampTo(MAX).
   int preferred_width = std::min(
       std::max(MinimumNonemptyWidth(), container_width_ - resize_amount_),
       IconCountToWidth(-1, false));
@@ skipping 89 matching lines @@
 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();
 
-  // Figure out where to display the indicator.  This is a complex calculation:
+  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:

[Devlin, 2014/09/16 19:28:35]

The body of this if is just the indented version of 453 - 515.

 
-  // 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();
+    // 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()) - ToolbarView::kStandardSpacing;
 
-  // If we're right-to-left, we flip the 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;
+    // Next, figure out what row we're on. This only matters for overflow mode,
+    // but the calculation is the same for both.
+    row_index = event.y() / IconHeight();
 
-  // 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();
+    // Sanity check - we should never be on a different row in the main
+    // container.
+    DCHECK(in_overflow_mode() || row_index == 0);
 
-  // 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
+    // to place the indicator in front of icon n when the cursor is between the
+    // midpoints of icons (n - 1) and n.  To do this we take the offset into the
+    // icon area and transform it as follows:
+    //
+    // Real icon area:
+    //   0   a     *  b        c
+    //   |   |        |        |
+    //   |[IC|ON]  [IC|ON]  [IC|ON]
+    // We want to be before icon 0 for 0 < x <= a, icon 1 for a < x <= b, etc.
+    // Here the "*" represents the offset into the icon area, and since it's
+    // between a and b, we want to return "1".
+    //
+    // Transformed "icon area":
+    //   0        a     *  b        c
+    //   |        |        |        |
+    //   |[ICON]  |[ICON]  |[ICON]  |
+    // If we shift both our offset and our divider points later by half an icon
+    // 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) +
+        kItemSpacing) / IconWidth(true);
 
-  // Next, we determine which icon to place the indicator in front of.  We want
-  // to place the indicator in front of icon n when the cursor is between the
-  // midpoints of icons (n - 1) and n.  To do this we take the offset into the
-  // icon area and transform it as follows:
-  //
-  // Real icon area:
-  //   0   a     *  b        c
-  //   |   |        |        |
-  //   |[IC|ON]  [IC|ON]  [IC|ON]
-  // We want to be before icon 0 for 0 < x <= a, icon 1 for a < x <= b, etc.
-  // Here the "*" represents the offset into the icon area, and since it's
-  // between a and b, we want to return "1".
-  //
-  // Transformed "icon area":
-  //   0        a     *  b        c
-  //   |        |        |        |
-  //   |[ICON]  |[ICON]  |[ICON]  |
-  // If we shift both our offset and our divider points later by half an icon
-  // 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) +
-      kItemSpacing) / IconWidth(true);
+    // We need to figure out how many icons are visible on the relevant row.
+    // In the main container, this will just be the visible actions.
+    int visible_icons_on_row = VisibleBrowserActionsAfterAnimation();
+    if (in_overflow_mode()) {
+      // If this is the final row of the overflow, then this is the remainder of
+      // visible icons. Otherwise, it's a full row (kIconsPerRow).
+      visible_icons_on_row =
+          row_index ==
+              static_cast<size_t>(visible_icons_on_row /
+                                  icons_per_overflow_menu_row_) ?
+                  visible_icons_on_row % icons_per_overflow_menu_row_ :
+                  icons_per_overflow_menu_row_;
+    }
 
-  // We need to figure out how many icons are visible on the relevant row.
-  // In the main container, this will just be the visible actions.
-  int visible_icons_on_row = VisibleBrowserActionsAfterAnimation();
-  if (in_overflow_mode()) {
-    // If this is the final row of the overflow, then this is the remainder of
-    // visible icons. Otherwise, it's a full row (kIconsPerRow).
-    visible_icons_on_row =
-        row_index ==
-            static_cast<size_t>(visible_icons_on_row /
-                                icons_per_overflow_menu_row_) ?
-                visible_icons_on_row % icons_per_overflow_menu_row_ :
-                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".
+    before_icon_in_row =
+        std::min(std::max(before_icon_unclamped, 0), visible_icons_on_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 =
-      std::min(std::max(before_icon_unclamped, 0), visible_icons_on_row);
-
   if (!drop_position_.get() ||
       !(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;
 }
 
@@ skipping 10 matching lines @@
     return ui::DragDropTypes::DRAG_NONE;
 
   // Make sure we have the same view as we started with.
   DCHECK_EQ(browser_action_views_[data.index()]->extension()->id(),
             data.id());
   DCHECK(model_);
 
   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();

[Peter Kasting, 2014/09/16 21:40:54]

It looks like now |i| is going to be based on the total number of browser
actions in both containers rather than just the number in the overflow
container.  But none of the other ode here changed.  That seems worrisome?

[Devlin, 2014/09/16 21:58:29]

Line 551 ensures that we're in the overflow container here.

[Peter Kasting, 2014/09/16 22:08:49]

I know.  I'm saying, the old code here called GetFirstVisibleIconIndex() which
would always return 0.  Now this code calls something which is presumably
guaranteed to be nonzero.  So the meaning of |i| has changed, but none of the
other code here appears to have changed in parallel.

[Devlin, 2014/09/16 22:10:39]

GetFirstVisibleIconIndex() would always return
main_container_->VisibleBrowserActionsAfterAnimation() for the overflow
container, and always return 0 for the main container.  Where we call it here is
for the overflow container, so VisibleBrowserActionsAfterAnimation() is correct.

Right?

[Peter Kasting, 2014/09/16 22:15:57]

Oh, I'm dumb, I mentally reversed the contents of GetFirstVisibleIconIndex(). 
Sorry...

[Devlin, 2014/09/16 22:25:37]

Happens to all of us. :)

   // |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
   // the left of the drop indicator, so we subtract one.
   // * Well, it can also point to the end, but not when dragging to the left. :)
   if (i > data.index())
     --i;
 
   if (profile_->IsOffTheRecord())
     i = model_->IncognitoIndexToOriginal(i);
@@ skipping 138 matching lines @@
 extensions::ActiveTabPermissionGranter*
     BrowserActionsContainer::GetActiveTabPermissionGranter() {
   content::WebContents* web_contents =
       browser_->tab_strip_model()->GetActiveWebContents();
   if (!web_contents)
     return NULL;
   return extensions::TabHelper::FromWebContents(web_contents)->
       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;
 }
 
 void BrowserActionsContainer::TestSetIconVisibilityCount(size_t icons) {
   model_->SetVisibleIconCountForTest(icons);
 }
 
 void BrowserActionsContainer::OnPaint(gfx::Canvas* canvas) {
   // If the views haven't been initialized yet, wait for the next call to
   // paint (one will be triggered by entering highlight mode).
   if (model_->is_highlighting() && !browser_action_views_.empty()) {
     views::Painter::PaintPainterAt(
         canvas, highlight_painter_.get(), GetLocalBounds());
   }
 
   // TODO(sky/glen): Instead of using a drop indicator, animate the icons while
   // dragging (like we do for tab dragging).
   if (drop_position_.get()) {
     // The two-pixel width drop indicator.
     static const int kDropIndicatorWidth = 2;
 
     // 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() +
-        (drop_position_->icon_in_row * IconWidth(true));
+    int drop_icon_x = ToolbarView::kStandardSpacing +

[Peter Kasting, 2014/09/16 21:40:54]

This seems very different than the old code.  Why is it correct?

[Devlin, 2014/09/16 21:58:29]

Before, we would say "The start of the first visible icon" because, when it was
written, we started the overflow and main containers at different points (IIRC,
overflow container had slightly different padding?).  This has changed, and now
each has only padding (ToolbarView::kStandardSpacing), followed by the icons.

+                      (drop_position_->icon_in_row * IconWidth(true));

[Peter Kasting, 2014/09/16 21:40:54]

Nit: Previous indenting was correct

[Devlin, 2014/09/16 21:58:29]

changed back.

     // Next, find the space before the drop icon. This will either be
     // kItemSpacing or ToolbarView::kStandardSpacing, depending on whether this
     // is the first icon.
     // NOTE: Right now, these are the same. But let's do this right for if they
     // ever aren't.
     int space_before_drop_icon = drop_position_->icon_in_row == 0 ?
         ToolbarView::kStandardSpacing : kItemSpacing;
     // Now place the drop indicator halfway between this and the end of the
     // previous icon.  If there is an odd amount of available space between the
     // two icons (or the icon and the address bar) after subtracting the drop
@@ skipping 386 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;
 }