Rework app list item drag zones.

ui/app_list/views/apps_grid_view.h

Index: ui/app_list/views/apps_grid_view.h
diff --git a/ui/app_list/views/apps_grid_view.h b/ui/app_list/views/apps_grid_view.h
index 9cf935fd618ebea2bb794dab6fd806f61e711b43..6c9e92002a261998ea857ac121ba67754e69b3da 100644
--- a/ui/app_list/views/apps_grid_view.h
+++ b/ui/app_list/views/apps_grid_view.h
@@ -235,6 +235,12 @@ class APP_LIST_EXPORT AppsGridView : public views::View,
     bool operator!=(const Index& other) const {
       return page != other.page || slot != other.slot;
     }
+    bool operator<(const Index& other) const {
+      if (page == other.page)

[tapted, 2014/09/11 08:08:55]

optional-nit: This is fine, but I usually write these like

if (page != other.page)
  return page < other.page;

return slot < other.slot;


it means if you have more than 2 axes you don't have to nest things, and you
don't have to mix axes in each condition/statement.

[calamity, 2014/09/15 03:26:11]

Done.

+        return slot < other.slot;
+
+      return page < other.page;
+    }
 
     int page;  // Which page an item view is on.
     int slot;  // Which slot in the page an item view is in.
@@ -266,6 +272,9 @@ class APP_LIST_EXPORT AppsGridView : public views::View,
   Index GetIndexOfView(const views::View* view) const;
   views::View* GetViewAtIndex(const Index& index) const;
 
+  // Gets the index of the AppListItemView in the view model.

[tapted, 2014/09/11 08:08:55]

`in the view model.` -> `at the end of the view model.`

[calamity, 2014/09/15 03:26:11]

Done.

+  Index GetLastViewIndex() const;
+
   void MoveSelected(int page_delta, int slot_x_delta, int slot_y_delta);
 
   void CalculateIdealBounds();
@@ -390,31 +399,17 @@ class APP_LIST_EXPORT AppsGridView : public views::View,
   // dropped into it.
   bool CanDropIntoTarget(const Index& drop_target);
 
-  // Calculates the visual index of the nearest tile for which |drag_view_|
-  // enters either its re-ordering or folder dropping circle.
-  void CalculateNearestTileForDragView();
-
-  // Calculates |nearest_tile| in which |vertex| of the |drag_view| is
-  // enclosed.
-  // *|nearest_tile| and *|d_min| will be updated based on the calculation.
-  // *|d_min| is the distance between |nearest_tile| and |drag_view_|.
-  void CalculateNearestTileForVertex(
-      const gfx::Point& vertex, Index* nearest_tile, int* d_min);
-
-  // Returns the bounds of the tile in which |point| is enclosed if there
-  // is a valid item sits on the tile.
-  gfx::Rect GetTileBoundsForPoint(const gfx::Point& point, Index* tile_index);
-
   // Returns the size of the entire tile grid.
   gfx::Size GetTileGridSize() const;
 
-  // Gets the expected bounds of a tile located at |row| and |col| on the
-  // current page.
-  gfx::Rect GetExpectedTileBounds(int row, int col) const;
+  // Returns the slot number which the given |point| falls into.

[tapted, 2014/09/11 08:08:55]

nit: This comment suggests to me that if |point| is not over a slot it will
return -1 or something. Still reading through the CL. I came back: could this
return an AppsGridView::Index with page == current page? - might neaten things
up a bit

[calamity, 2014/09/15 03:26:11]

Returns an Index, changed description.

+  int GetNearestTileSlotForPoint(const gfx::Point& point);

[tapted, 2014/09/11 08:08:55]

nit: declare const?

[calamity, 2014/09/15 03:26:11]

Done.

 
-  // Returns true if the slot of |index| is the last possible slot to drop
-  // an item, i.e. first empty slot next to the last item on the last page.
-  bool IsLastPossibleDropTarget(const Index& index) const;
+  // Gets the bounds of the tile located at |slot| on current page.
+  gfx::Rect GetExpectedTileBounds(int slot) const;

[tapted, 2014/09/11 08:08:55]

maybe take an AppsGridView::Index ? looks like it's always called with
index.slot

[calamity, 2014/09/15 03:26:11]

I think slot is better since using an Index kind of implies that this will
consider the page when calculating the bounds.

+
+  // Gets the bounds of the tile located at |row| and |col| on current page.

[tapted, 2014/09/11 08:08:55]

nit: on -> on the

[calamity, 2014/09/15 03:26:11]

Done.

+  gfx::Rect GetExpectedTileBounds(int row, int col) const;
 
   // Gets the item view located at |slot| on the current page. If there is
   // no item located at |slot|, returns NULL.
@@ -513,8 +508,17 @@ class APP_LIST_EXPORT AppsGridView : public views::View,
 #endif
 
   Pointer drag_pointer_;
-  Index reorder_drop_target_;
+
+  // The index where a folder drop is being attempted.
   Index folder_drop_target_;
+
+  // The index where a reorder is being attempted.
+  Index reorder_drop_target_;
+
+  // The index where a reorder's blank placeholder is located.
+  Index reorder_placeholder_;

[tapted, 2014/09/11 08:08:55]

Can you say more about this in the comment - I'm not sure why the placeholder is
needed. And, e.g., how is this different to drag_view_init_index_?

[calamity, 2014/09/15 03:26:11]

Done.

+
+  // The type of drop being attempted.
   DropAttempt drop_attempt_;
 
   // Timer for re-ordering the |drop_target_| and |drag_view_|.