Refactor app list max folder logic.

ui/app_list/views/apps_grid_view.cc

Index: ui/app_list/views/apps_grid_view.cc
diff --git a/ui/app_list/views/apps_grid_view.cc b/ui/app_list/views/apps_grid_view.cc
index 4971720fe9d2ef355cfeb7fe8b9bf693b1ff3444..0e0d388e78dc321f3a73f29f5016110d1c1337b2 100644
--- a/ui/app_list/views/apps_grid_view.cc
+++ b/ui/app_list/views/apps_grid_view.cc
@@ -212,7 +212,7 @@ class ItemMoveAnimationDelegate : public gfx::AnimationDelegate {
   DISALLOW_COPY_AND_ASSIGN(ItemMoveAnimationDelegate);
 };
 
-// Returns true if the |item| is an folder item.
+// Returns true if the |item| is a folder item.
 bool IsFolderItem(AppListItem* item) {
   return (item->GetItemType() == AppListFolderItem::kItemType);
 }
@@ -1393,20 +1393,39 @@ void AppsGridView::CalculateDropTarget() {
 
 bool AppsGridView::CalculateFolderDropTarget(const gfx::Point& point,
                                              Index* drop_target) const {
+  // Folders can't be dropped into other folders.
+  if (IsFolderItem(drag_view_->item()))
+    return false;
+
+  // A folder drop shouldn't happen on the reorder placeholder since that would
+  // be merging an item with itself.
   Index nearest_tile_index(GetNearestTileIndexForPoint(point));
+  if (GetLastViewIndex() < nearest_tile_index ||

[Matt Giuca, 2014/10/02 06:08:59]

I don't understand where this condition (GetLastViewIndex() <
nearest_tile_index) came from or what it means.

[calamity, 2014/10/03 03:37:36]

Wups. Roundabout way of checking index validity. Fixed.

+      nearest_tile_index == reorder_placeholder_) {
+    return false;
+  }
+
   int distance_to_tile_center =
       (point - GetExpectedTileBounds(nearest_tile_index.slot).CenterPoint())
           .Length();
-  if (nearest_tile_index != reorder_placeholder_ &&
-      distance_to_tile_center < kFolderDroppingCircleRadius &&
-      !IsFolderItem(drag_view_->item()) &&
-      CanDropIntoTarget(nearest_tile_index)) {
-    *drop_target = nearest_tile_index;
-    DCHECK(IsValidIndex(*drop_target));
-    return true;
+  if (distance_to_tile_center > kFolderDroppingCircleRadius)

[Matt Giuca, 2014/10/02 06:08:59]

FYI, you are slightly changing the behaviour here. (Previously, if
dist_to_tile_center == kFDCR, it would be false, but now it is true.)

Perhaps this is an improvement, but let's be sure we want this.

[calamity, 2014/10/03 03:37:36]

I think radii of drop zones should be inclusive of their borders. The old code
provided 2 * kFDCR - 2 units of drop diameter which is weird.

[Matt Giuca, 2014/10/09 17:25:18]

Acknowledged.

+    return false;
+
+  AppListItemView* target_view =

[Matt Giuca, 2014/10/02 06:08:59]

I'm not sure I agree with inlining CanDropIntoTarget here. What was the
rationale?

[calamity, 2014/10/03 03:37:37]

It's only used in one place and the logic should be display based. So that means
we can have a method which is current-page-based (which kinda kills the
reusability of the method and makes it uncomfortably situational) or we can pass
the entire index and expect callers to figure out that the model doesn't match
the view (which is the kind of pitfall that led to the bug in the first place).

Keeping the view grabbing logic here is the safest way to avoid those problems.
Maybe it's worth having an CanAcceptFolderDrop() on AppListItemView if we want a
more reusable place for this code.

[Matt Giuca, 2014/10/09 17:25:18]

Acknowledged.

+      GetViewDisplayedAtSlotOnCurrentPage(nearest_tile_index.slot);

[Matt Giuca, 2014/10/02 06:08:59]

You removed the NULL-check here?

[calamity, 2014/10/03 03:37:37]

Hmmmmm. I _think_ this can't be NULL. But maybe it can...

I'll put it back.

+  AppListItem* target_item = target_view->item();
+
+  // Items can only be dropped into non-folders (which have no children) or
+  // folders that have fewer than the max allowed items.
+  // The OEM folder does not allow drag/drop of other items into it.
+  if (target_item->ChildItemCount() >= kMaxFolderItems ||
+      IsOEMFolderItem(target_item)) {
+    return false;
   }
 
-  return false;
+  *drop_target = nearest_tile_index;
+  DCHECK(IsValidIndex(*drop_target));
+  return true;
 }
 
 void AppsGridView::CalculateReorderDropTarget(const gfx::Point& point,
@@ -2084,20 +2103,6 @@ bool AppsGridView::EnableFolderDragDropUI() {
   return model_->folders_enabled() && !folder_delegate_;
 }
 
-bool AppsGridView::CanDropIntoTarget(const Index& drop_target) const {
-  AppListItemView* target_view =
-      GetViewDisplayedAtSlotOnCurrentPage(drop_target.slot);
-  if (!target_view)
-    return false;
-
-  AppListItem* target_item = target_view->item();
-  // Items can be dropped into non-folders (which have no children) or folders
-  // that have fewer than the max allowed items.
-  // OEM folder does not allow to drag/drop other items in it.
-  return target_item->ChildItemCount() < kMaxFolderItems &&
-         !IsOEMFolderItem(target_item);
-}
-
 AppsGridView::Index AppsGridView::GetNearestTileIndexForPoint(
     const gfx::Point& point) const {
   gfx::Rect bounds = GetContentsBounds();