Misc. cleanup:

chrome/browser/ui/views/tabs/tab_drag_controller.cc

 // Copyright (c) 2012 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/tabs/tab_drag_controller.h"
 
 #include <math.h>
 #include <set>
 
 #include "ash/accelerators/accelerator_commands.h"
@@ skipping 778 matching lines @@
   for (size_t i = 0; i < drag_data_.size(); ++i)
     tabs[i] = drag_data_[i].attached_tab;
 
   bool did_layout = false;
   // Update the model, moving the WebContents from one index to another. Do this
   // only if we have moved a minimum distance since the last reorder (to prevent
   // jitter) or if this the first move and the tabs are not consecutive.
   if ((abs(point_in_screen.x() - last_move_screen_loc_) > threshold ||
         (initial_move_ && !AreTabsConsecutive()))) {
     TabStripModel* attached_model = GetModel(attached_tabstrip_);
-    gfx::Rect bounds = GetDraggedViewTabStripBounds(dragged_view_point);
-    int to_index = GetInsertionIndexForDraggedBounds(bounds);
+    int to_index = GetInsertionIndexForDraggedBounds(
+        GetDraggedViewTabStripBounds(dragged_view_point));

[Peter Kasting, 2014/06/17 03:16:18]

I didn't think the old code was bad, but while working on this code for other
reasons I found myself trying to discover whether |bounds| was used elsewhere,
and figured I might as well remove that question if doing so didn't add any more
code.

     bool do_move = true;
     // While dragging within a tabstrip the expectation is the insertion index
     // is based on the left edge of the tabs being dragged. OTOH when dragging
     // into a new tabstrip (attaching) the expectation is the insertion index is
     // based on the cursor. This proves problematic as insertion may change the
     // size of the tabs, resulting in the index calculated before the insert
     // differing from the index calculated after the insert. To alleviate this
     // the index is chosen before insertion, and subsequently a new index is
     // only used once the mouse moves enough such that the index changes based
     // on the direction the mouse moved relative to |attach_x_| (smaller
@@ skipping 186 matching lines @@
 
     // Figure out where to insert the tab based on the bounds of the dragged
     // representation and the ideal bounds of the other Tabs already in the
     // strip. ("ideal bounds" are stable even if the Tabs' actual bounds are
     // changing due to animation).
     gfx::Point tab_strip_point(point_in_screen);
     views::View::ConvertPointFromScreen(attached_tabstrip_, &tab_strip_point);
     tab_strip_point.set_x(
         attached_tabstrip_->GetMirroredXInView(tab_strip_point.x()));
     tab_strip_point.Offset(0, -mouse_offset_.y());
-    gfx::Rect bounds = GetDraggedViewTabStripBounds(tab_strip_point);
-    int index = GetInsertionIndexForDraggedBounds(bounds);
+    int index = GetInsertionIndexForDraggedBounds(
+        GetDraggedViewTabStripBounds(tab_strip_point));
     attach_index_ = index;
     attach_x_ = tab_strip_point.x();
     base::AutoReset<bool> setter(&is_mutating_, true);
     for (size_t i = 0; i < drag_data_.size(); ++i) {
       int add_types = TabStripModel::ADD_NONE;
       if (attached_tabstrip_->touch_layout_.get()) {
         // StackedTabStripLayout positions relative to the active tab, if we
         // don't add the tab as active things bounce around.
         DCHECK_EQ(1u, drag_data_.size());
         add_types |= TabStripModel::ADD_ACTIVE;
@@ skipping 240 matching lines @@
     } else if (dragged_bounds.x() >= left_half.x() &&
                dragged_bounds.x() < left_half.right()) {
       return i;
     }
   }
   return -1;
 }
 
 int TabDragController::GetInsertionIndexForDraggedBounds(
     const gfx::Rect& dragged_bounds) const {
+  // If the strip has no tabs, the only position to insert at is 0.
+  const int tab_count = attached_tabstrip_->tab_count();
+  if (!tab_count)
+    return 0;
+
   int index = -1;
   if (attached_tabstrip_->touch_layout_.get()) {
     index = GetInsertionIndexForDraggedBoundsStacked(dragged_bounds);
     if (index != -1) {
       // Only move the tab to the left/right if the user actually moved the
       // mouse that way. This is necessary as tabs with stacked tabs
       // before/after them have multiple drag positions.
       int active_index = attached_tabstrip_->touch_layout_->active_index();
       if ((index < active_index &&
            (mouse_move_direction_ & kMovedMouseLeft) == 0) ||
           (index > active_index &&
            (mouse_move_direction_ & kMovedMouseRight) == 0)) {
         index = active_index;
       }
     }
   } else {
     index = GetInsertionIndexFrom(dragged_bounds, 0, 1);
   }
   if (index == -1) {
-    int tab_count = attached_tabstrip_->tab_count();
-    int right_tab_x = tab_count == 0 ? 0 :
+    const int last_tab_right =
         attached_tabstrip_->ideal_bounds(tab_count - 1).right();
-    if (dragged_bounds.right() > right_tab_x) {
-      index = GetModel(attached_tabstrip_)->count();
-    } else {
-      index = 0;
-    }
+    index = (dragged_bounds.right() > last_tab_right) ? tab_count : 0;

[Peter Kasting, 2014/06/17 03:16:18]

Note that this assumes attached_tabstrip_->tab_count() and
GetModel(attached_tabstrip_)->count() are equal.  That's correct, right?

[sky, 2014/06/17 15:42:42]

Yes, they should be equal (with the exception of the observer list ordering bugs
you encountered).

   }
 
   if (!drag_data_[0].attached_tab) {
     // If 'attached_tab' is NULL, it means we're in the process of attaching and
     // don't need to constrain the index.
     return index;
   }
 
   int max_index = GetModel(attached_tabstrip_)->count() -
       static_cast<int>(drag_data_.size());
@@ skipping 487 matching lines @@
       break;
     }
     default:
       break; // Nothing to do for DETACH_ABOVE_OR_BELOW.
   }
 
   // To account for the extra vertical on restored windows that is absent on
   // maximized windows, add an additional vertical offset extracted from the tab
   // strip.
   if (source->GetWidget()->IsMaximized())
-    new_bounds.Offset(0, -source->button_v_offset());
+    new_bounds.Offset(0, -source->kNewTabButtonVerticalOffset);
   return new_bounds;
 }
 
 void TabDragController::AdjustBrowserAndTabBoundsForDrag(
     int last_tabstrip_width,
     const gfx::Point& point_in_screen,
     std::vector<gfx::Rect>* drag_bounds) {
   attached_tabstrip_->InvalidateLayout();
   attached_tabstrip_->DoLayout();
   const int dragged_tabstrip_width = attached_tabstrip_->tab_area_width();
@@ skipping 107 matching lines @@
        it != browser_list->end(); ++it) {
     if ((*it)->tab_strip_model()->empty())
       exclude.insert((*it)->window()->GetNativeWindow());
   }
 #endif
   return GetLocalProcessWindowAtPoint(host_desktop_type_,
                                       screen_point,
                                       exclude);
 
 }