Refactor app list max folder logic.

Refactor app list max folder logic.

This CL refactors the logic to fit all the folder
dropping logic in CalculateFolderDropTarget().

BUG=419642
Committed: https://crrev.com/91d3b69357342a5d47595e81791d6b56aac531b6
Cr-Commit-Position: refs/heads/master@{#300068}

[calamity, 2014-10-02 05:12:19]

Patchset #1 (id:1) has been deleted

[calamity, 2014-10-02 05:13:28]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[calamity, 2014-10-02 05:13:29]

This is much nicer.

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

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1403: if (GetLastViewIndex() <
nearest_tile_index ||
I don't understand where this condition (GetLastViewIndex() <
nearest_tile_index) came from or what it means.

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1411: if (distance_to_tile_center >
kFolderDroppingCircleRadius)
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.

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1414: AppListItemView* target_view =
I'm not sure I agree with inlining CanDropIntoTarget here. What was the
rationale?

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1415:
GetViewDisplayedAtSlotOnCurrentPage(nearest_tile_index.slot);
You removed the NULL-check here?

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

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1403: if (GetLastViewIndex() <
nearest_tile_index ||
On 2014/10/02 06:08:59, Matt Giuca wrote:
> I don't understand where this condition (GetLastViewIndex() <
> nearest_tile_index) came from or what it means.

Wups. Roundabout way of checking index validity. Fixed.

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1411: if (distance_to_tile_center >
kFolderDroppingCircleRadius)
On 2014/10/02 06:08:59, Matt Giuca wrote:
> 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.

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.

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1414: AppListItemView* target_view =
On 2014/10/02 06:08:59, Matt Giuca wrote:
> I'm not sure I agree with inlining CanDropIntoTarget here. What was the
> rationale?

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.

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1415:
GetViewDisplayedAtSlotOnCurrentPage(nearest_tile_index.slot);
On 2014/10/02 06:08:59, Matt Giuca wrote:
> You removed the NULL-check here?

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

I'll put it back.

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

lgtm.

Please change the CL description removing the reference to tests (since you
moved the tests to a different CL).

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1411: if (distance_to_tile_center >
kFolderDroppingCircleRadius)
On 2014/10/03 03:37:36, calamity wrote:
> On 2014/10/02 06:08:59, Matt Giuca wrote:
> > 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.
> 
> 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.

Acknowledged.

https://codereview.chromium.org/614213004/diff/20001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1414: AppListItemView* target_view =
On 2014/10/03 03:37:37, calamity wrote:
> On 2014/10/02 06:08:59, Matt Giuca wrote:
> > I'm not sure I agree with inlining CanDropIntoTarget here. What was the
> > rationale?
> 
> 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.

Acknowledged.

[calamity, 2014-10-10 20:39:53]

Changed CL description.

[calamity, 2014-10-10 20:40:01]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-10-10 20:41:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/614213004/60001

[commit-bot: I haz the power, 2014-10-10 21:40:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-10 21:40:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2014-10-16 23:55:35]

Patchset #3 (id:60001) has been deleted

[calamity, 2014-10-16 23:55:59]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-10-16 23:56:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/614213004/270001

[commit-bot: I haz the power, 2014-10-17 01:18:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-17 01:18:47]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2014-10-17 04:57:10]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-10-17 04:57:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/614213004/290001

[commit-bot: I haz the power, 2014-10-17 06:09:01]

Committed patchset #4 (id:290001)

[commit-bot: I haz the power, 2014-10-17 06:09:48]

Patchset 4 (id:??) landed as
https://crrev.com/91d3b69357342a5d47595e81791d6b56aac531b6
Cr-Commit-Position: refs/heads/master@{#300068}