Adjust BrowserActionsContainer drag and drop to work for overflow

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 faae39a6e02f785a54441e2e9e704e569ff4fdee..4cd3d3c523ce2d425f4edf5eb5ad85d9bf76c63d 100644
--- a/chrome/browser/ui/views/toolbar/browser_actions_container.cc
+++ b/chrome/browser/ui/views/toolbar/browser_actions_container.cc
@@ -102,6 +102,25 @@ class ChevronMenuButton : public views::MenuButton {
 // static
 bool BrowserActionsContainer::disable_animations_during_testing_ = false;

[Peter Kasting, 2014/07/21 20:24:08]

Nit: Put this below the "BrowserActionsContainer" divider comment below.

[Devlin, 2014/07/22 17:30:46]

Done.

 
+struct BrowserActionsContainer::DropPosition {

[Peter Kasting, 2014/07/21 20:24:08]

Nit: Consider a "// BrowserActionsContainer::DropPosition ---..." or similar
divider about this class (up to you)

[Devlin, 2014/07/22 17:30:46]

SGTM. Done.

+  DropPosition(size_t row, size_t icon_in_row);
+  ~DropPosition();

[Peter Kasting, 2014/07/21 20:24:08]

Nit: Is declaring this destructor necessary?

[Devlin, 2014/07/22 17:30:47]

For some reason, I thought it was chrome/google style.  But I can't find a rule
indicating it is, so taken out.

+
+  // The (0-indexed) row into which the action will be dropped.
+  size_t row;
+
+  // The (0-indexed) icon in the row before the action will be dropped.
+  size_t icon_in_row;
+};
+
+BrowserActionsContainer::DropPosition::DropPosition(
+    size_t row, size_t icon_in_row)
+    : row(row), icon_in_row(icon_in_row) {
+}
+
+BrowserActionsContainer::DropPosition::~DropPosition() {
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserActionsContainer
 
@@ -123,7 +142,6 @@ BrowserActionsContainer::BrowserActionsContainer(
       suppress_chevron_(false),
       resize_amount_(0),
       animation_target_size_(0),
-      drop_indicator_position_(-1),
       task_factory_(this),
       show_menu_task_factory_(this) {
   set_id(VIEW_ID_BROWSER_ACTION_TOOLBAR);
@@ -359,7 +377,7 @@ void BrowserActionsContainer::Layout() {
       BrowserActionView* view = browser_action_views_[i];
       size_t index = i - main_container_->VisibleBrowserActionsAfterAnimation();
       int row_index = static_cast<int>(index) / kIconsPerMenuRow;
-      int x = (index * IconWidth(true)) -
+      int x = kItemSpacing + (index * IconWidth(true)) -

[Peter Kasting, 2014/07/21 20:24:08]

So, we have kItemSpacing to the left of all the icons, but not between each pair
of icons?

[Devlin, 2014/07/22 17:30:46]

We do have it between each pair of icons, but IconWidth() is somewhat
unfortunately done. The bool passed in indicates "include padding", which, if
true, returns the width of the icon + kItemSpacing.  The extra kItemSpacing here
is to account for the spacing before each icon (e.g., for the 0th index, we
still have 1 spacing, for the 1st index, we have 2 spacing, etc).

           (row_index * IconWidth(true) * kIconsPerMenuRow);
       gfx::Rect rect_bounds(
           x, IconHeight() * row_index, icon_width, IconHeight());
@@ -407,19 +425,26 @@ int BrowserActionsContainer::OnDragUpdated(
   }
   StopShowFolderDropMenuTimer();
 
-  // TODO(devlin): This calculation needs to take 'overflow' mode into account
-  // once the wrench menu becomes a drag target for browser action icons.
-
   // 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.
-  int width_before_icons = ToolbarView::kStandardSpacing;
-  if (chevron_ && base::i18n::IsRTL()) {
-    width_before_icons +=
-        chevron_->GetPreferredSize().width() + kChevronSpacing;
-  }
-  int offset_into_icon_area = event.x() - width_before_icons;
+  // 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.
+  int offset_into_icon_area =
+    (base::i18n::IsRTL() ? width() - event.x() : event.x()) -

[Peter Kasting, 2014/07/21 20:24:08]

Nit: Use "GetMirroredXInView(event.x())"; indent 2, not 4

[Devlin, 2014/07/22 17:30:46]

Done.

+    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.
+  int 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);
 
   // 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
@@ -445,25 +470,33 @@ int BrowserActionsContainer::OnDragUpdated(
   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 == (visible_icons_on_row / kIconsPerMenuRow) ?
+            visible_icons_on_row % kIconsPerMenuRow :
+            kIconsPerMenuRow;
+  }
+
   // 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".
-  int before_icon = std::min(std::max(before_icon_unclamped, 0),
-                             static_cast<int>(VisibleBrowserActions()));
+  int before_icon_in_row =
+      std::min(std::max(before_icon_unclamped, 0), visible_icons_on_row);
 
-  // Now we convert back to a pixel offset into the container.  We want to place
-  // the center of the drop indicator at the midpoint of the space before our
-  // chosen icon.
-  SetDropIndicator(width_before_icons + (before_icon * IconWidth(true)) -
-      (kItemSpacing / 2));
+  SetDropPosition(row_index, before_icon_in_row);
 
   return ui::DragDropTypes::DRAG_MOVE;
 }
 
 void BrowserActionsContainer::OnDragExited() {
   StopShowFolderDropMenuTimer();
-  drop_indicator_position_ = -1;
+  ResetDropPosition();
   SchedulePaint();
 }
 
@@ -478,18 +511,10 @@ int BrowserActionsContainer::OnPerformDrop(
             data.id());
   DCHECK(model_);
 
-  size_t i = 0;
-  for (; i < browser_action_views_.size(); ++i) {
-    int view_x = browser_action_views_[i]->GetMirroredBounds().x();
-    if (!browser_action_views_[i]->visible() ||
-        (base::i18n::IsRTL() ? (view_x < drop_indicator_position_) :
-            (view_x >= drop_indicator_position_))) {
-      // We have reached the end of the visible icons or found one that has a
-      // higher x position than the drop point.
-      break;
-    }
-  }
-
+  size_t i =
+      drop_position_->row * kIconsPerMenuRow + drop_position_->icon_in_row;
+  if (in_overflow_mode())
+    i += GetFirstVisibleIconIndex();
   // |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
@@ -690,6 +715,10 @@ bool BrowserActionsContainer::ShowPopup(const extensions::Extension* extension,
   return false;
 }
 
+size_t BrowserActionsContainer::GetFirstVisibleIconIndex() const {
+  return in_overflow_mode() ? model_->GetVisibleIconCount() : 0;
+}
+
 void BrowserActionsContainer::HidePopup() {
   // Remove this as an observer and clear |popup_| and |popup_button_| here,
   // since we might change them before OnWidgetDestroying() gets called.
@@ -727,14 +756,39 @@ void BrowserActionsContainer::OnPaint(gfx::Canvas* canvas) {
 
   // TODO(sky/glen): Instead of using a drop indicator, animate the icons while
   // dragging (like we do for tab dragging).
-  if (drop_indicator_position_ > -1) {
+  if (drop_position_.get()) {
     // The two-pixel width drop indicator.
     static const int kDropIndicatorWidth = 2;
-    gfx::Rect indicator_bounds(
-        drop_indicator_position_ - (kDropIndicatorWidth / 2),
-        0,
-        kDropIndicatorWidth,
-        height());
+
+    // Convert back to a pixel offset into the container. We want to place
+    // the center of the drop indicator at the midpoint of the space before
+    // the drop position's referenced icon.
+    // This is:
+    //     The width before the first icon, plus
+    //     The width of all icons before the target icon in the row, minus
+    //     Half the spacing between the target icon and the previous icon, minus
+    //     Half the width of the drop indicator, so it's centered.
+    int drop_indicator_x =
+        browser_action_views_[GetFirstVisibleIconIndex()]->x() +
+        drop_position_->icon_in_row * IconWidth(true) -
+        kItemSpacing / 2 -
+        kDropIndicatorWidth / 2;
+
+    // Mirror the x if it's RTL. Why the extra -1? Because kItemSpacing is 3,
+    // so kItemSpacing / 2 = 1. In order to draw the drop indicator at the same
+    // point in RTL, we have to subtract the extra difference (otherwise it
+    // draws to far to the right).

[Peter Kasting, 2014/07/21 20:24:08]

Nit: to far -> too far

This logic doesn't really make sense to me, though.  This is mathematically
equivalent to just having not inserted the "kItemSpacing / 2" in the calculation
above (when we're RTL).  It seems like if there is a rounding issue here, we'd
want to shift things further right by 1 px in RTL, not further left.

Maybe you can give some screnshots to illustrate the issue here?

[Devlin, 2014/07/22 17:30:46]

Yeah, it's confusing. :)  So, in LTR, we shift things "closer to the icon"
(because we take kItemSpacing / 2 = 3 / 2 = 1, so we only move one pixel away
from the icon).  When we mirror this to RTL, to get the same effect, "closer to
the icon" requires an offset of 2 (because it's mirrored), so we basically want
kItemSpacing / 2 = 2.  Since we subtract kItemSpacing, to make up the
difference, we subtract 1.

And, of course, screenshots are worth 1000 words.
http://imgur.com/PtFGQ4f
The RTL is most apparently wrong without the -1 when dragging to the far right
(i.e., closer to the address bar) as the drop indicator actually gets cut off.

[Peter Kasting, 2014/07/22 18:21:11]

The screenshots are pretty clear that what you're doing makes sense.

It still boggles my mind, though.  When you say "Since we subtract kItemSpacing,
to make up the difference, we subtract 1", my response is, "But we subtract that
value before we mirror everything -- and mirroring flips all the signs.  So
effectively, after we mirror, we've 'added' kItemSpacing/2, and now we're
undoing that, rather than adding to it."

Another way of saying this is, if you replaced "kItemSpacing / 2" with "2",
eliminated the RTL-specific subtraction, and then ran in RTL mode, I'd expect
you to get a different result than your current code.  If I'm right, it suggests
your justification here is a bit wonky, and you're not really trying to subtract
"kItemSpacing / 2" in the first place, you really just have an LTR-specific "-1"
for some reason.

Consider testing the above theory and, if it's correct, thinking more deeply
about how the code and comments here should read.

[Devlin, 2014/07/22 20:29:32]

Ah!  You're quite right, there's a couple things wrong here (even in the old
code).

We should not be subtracting kDropIndicatorWidth / 2, but rather
kDropIndicatorWidth, as this is used to set the bounds (which need the starting
x, not the center).  We add in an extra pixel, because kItemSpacing is odd, and
it looks better a bit closer to the icon and farther from the address bar. 
Then, we *do* need an RTL correction, because FillRect doesn't use RTL bounds
(hence why we mirror in the first place).  Because of this, it fills LTR, and we
actually need to shift the bounds of the rect by kDropIndicatorWidth to
compensate.

+    if (base::i18n::IsRTL())
+      drop_indicator_x = width() - drop_indicator_x - 1;

[Peter Kasting, 2014/07/21 20:24:08]

Nit: MirroredXInView()

[Devlin, 2014/07/22 17:30:46]

Done.

+
+    // The y is simply the start of the row (which for the main container is
+    // always 0).
+    int drop_indicator_y = IconHeight() * drop_position_->row;
+
+    gfx::Rect indicator_bounds(drop_indicator_x,
+                               drop_indicator_y,
+                               kDropIndicatorWidth,
+                               IconHeight());
 
     // Color of the drop indicator.
     static const SkColor kDropIndicatorColor = SK_ColorBLACK;
@@ -945,7 +999,7 @@ void BrowserActionsContainer::StartShowFolderDropMenuTimer() {
 
 void BrowserActionsContainer::ShowDropFolder() {
   DCHECK(!overflow_menu_);
-  SetDropIndicator(-1);
+  ResetDropPosition();
   overflow_menu_ =
       new BrowserActionOverflowMenuController(this,
                                               browser_,
@@ -957,13 +1011,19 @@ void BrowserActionsContainer::ShowDropFolder() {
   overflow_menu_->RunMenu(GetWidget());
 }
 
-void BrowserActionsContainer::SetDropIndicator(int x_pos) {
-  if (drop_indicator_position_ != x_pos) {
-    drop_indicator_position_ = x_pos;
+void BrowserActionsContainer::SetDropPosition(size_t row, size_t icon_in_row) {

[Peter Kasting, 2014/07/21 20:24:08]

Nit: This is only called once.  It might be clearer to do this in the caller.

If you use my suggestion about storing this member by value instead of pointer
(see comments in .h), we could do something like:

  const DropPosition old_pos(drop_position_);
  drop_position_ = DropPosition(row, icon_in_row);
  if (drop_position_ != old_pos)
    SchedulePaint();

[Devlin, 2014/07/22 17:30:46]

Inlined.  See comment in the .h about the rest.

+  if (!drop_position_.get() ||
+      !(drop_position_->row == row &&
+        drop_position_->icon_in_row == icon_in_row)) {
+    drop_position_.reset(new DropPosition(row, icon_in_row));
     SchedulePaint();
   }
 }
 
+void BrowserActionsContainer::ResetDropPosition() {

[Peter Kasting, 2014/07/21 20:24:08]

I'm not convinced having this is preferable to just calling reset() directly
inline in the callers.

[Devlin, 2014/07/22 17:30:46]

Agreed - I put this in for parity with SetDropPosition, but it's really not
needed (especially if we inline SetDropPosition()).

+  drop_position_.reset();
+}
+
 int BrowserActionsContainer::IconCountToWidth(int icons,
                                               bool display_chevron) const {
   if (icons < 0)