Split app list drop target into reorder and folder drop targets.

Split app list drop target into reorder and folder drop targets.

This CL splits the common drop_target_ member of AppsGridView into
reorder_drop_target_ and folder_drop_target_. This more clearly shows
the purpose which a drop target is being used for.

This also slightly changes the behavior when dropping an app into a
full folder as the latest reorder drop target will be used for
reordering instead of the common drop target which would have pointed
at the folder.

BUG=411775
Committed: https://crrev.com/cc0485133fd3ec2ecb462c8bd1a4a1d5cd34faac
Cr-Commit-Position: refs/heads/master@{#294777}

[calamity, 2014-09-10 05:43:16]

Patchset #1 (id:1) has been deleted

[calamity, 2014-09-10 05:43:23]

Patchset #1 (id:20001) has been deleted

[calamity, 2014-09-10 06:16:08]

Patchset #1 (id:40001) has been deleted

[calamity, 2014-09-10 06:16:20]

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

[Matt Giuca, 2014-09-11 03:35:40]

Seems legit.

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

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1190: if (reorder_drop_target_ == view_index
&&
Can you add a comment saying what this does? 

Also, I'm pretty confused as to why you need to say "!EnableFolderDragDropUI ||
...". Surely if folder drag drop UI is disabled, drop_attempt_ will equal
DROP_FOR_REORDER, so there is no need for that first check.

Also, just to clarify: in the old code, the "then" branch of the inner if
statement did absolutely nothing, correct? So you're just simplifying this
statement? (Not changing behaviour.)

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:2124: // If |drag_view| is not entering the
re-order or fold dropping region of
nit: folder

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:516: Index reorder_drop_target_;
Comments here.

In particular, say that reorder_drop_target_ is only valid when drop_attempt_ is
DROP_FOR_REORDER, and so on.

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view_unittest.cc (right):

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_unittest.cc:411: // item.
Sorry, too hard to read... can you explain what this test *was* testing and why
you had to delete this?

Is this related to the bug you fixed (which I also don't understand from your CL
description). Was this testing broken behaviour?

[calamity, 2014-09-11 07:05:08]

Patchset #2 (id:80001) has been deleted

[calamity, 2014-09-11 07:05:17]

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

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1190: if (reorder_drop_target_ == view_index
&&
On 2014/09/11 03:35:39, Matt Giuca wrote:
> Can you add a comment saying what this does? 
> 
Done.
> Also, I'm pretty confused as to why you need to say "!EnableFolderDragDropUI
||
> ...". Surely if folder drag drop UI is disabled, drop_attempt_ will equal
> DROP_FOR_REORDER, so there is no need for that first check.
> 
This required setting the drop_attempt_ in the non folder path.
> Also, just to clarify: in the old code, the "then" branch of the inner if
> statement did absolutely nothing, correct? So you're just simplifying this
> statement? (Not changing behaviour.)

Yes.

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:2124: // If |drag_view| is not entering the
re-order or fold dropping region of
On 2014/09/11 03:35:39, Matt Giuca wrote:
> nit: folder

Done.

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:516: Index reorder_drop_target_;
On 2014/09/11 03:35:39, Matt Giuca wrote:
> Comments here.
> 
> In particular, say that reorder_drop_target_ is only valid when drop_attempt_
is
> DROP_FOR_REORDER, and so on.

Done.

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view_unittest.cc (right):

https://codereview.chromium.org/557673002/diff/60001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view_unittest.cc:411: // item.
On 2014/09/11 03:35:40, Matt Giuca wrote:
> Sorry, too hard to read... can you explain what this test *was* testing and
why
> you had to delete this?
> 
> Is this related to the bug you fixed (which I also don't understand from your
CL
> description). Was this testing broken behaviour?

Amended CL description as discussed.

[Matt Giuca, 2014-09-12 01:13:28]

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.cc:1393: bool use_page_button_hovering) {
Shouldn't this set drop_attempt_ to DROP_FOR_FOLDER if successful?

(Since it sets drop_attempt_ = DROP_FOR_NONE in other cases, and also since
CalculateDropTarget is now setting DROP_FOR_REORDER -- it seems logical to set
drop_attempt_ in all situations, rather than leaving it unchanged in the folder
case.)

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.h:294: // as drop target.
Rewrite this comment now that |drop_target_| has been split up, and also note
that this method will set |drop_attempt_| as well.

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.h:301: // dropping circle.
Ditto (and see my comments in this method's implementation).

[calamity, 2014-09-12 08:26:06]

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.cc:1393: bool use_page_button_hovering) {
On 2014/09/12 01:13:28, Matt Giuca wrote:
> Shouldn't this set drop_attempt_ to DROP_FOR_FOLDER if successful?
> 
> (Since it sets drop_attempt_ = DROP_FOR_NONE in other cases, and also since
> CalculateDropTarget is now setting DROP_FOR_REORDER -- it seems logical to set
> drop_attempt_ in all situations, rather than leaving it unchanged in the
folder
> case.)

I think this will be fine for now. This is all getting blown away soon anyhow.

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.h:294: // as drop target.
On 2014/09/12 01:13:28, Matt Giuca wrote:
> Rewrite this comment now that |drop_target_| has been split up, and also note
> that this method will set |drop_attempt_| as well.

Done.

https://codereview.chromium.org/557673002/diff/100001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.h:301: // dropping circle.
On 2014/09/12 01:13:28, Matt Giuca wrote:
> Ditto (and see my comments in this method's implementation).

Done.

[Matt Giuca, 2014-09-12 09:15:46]

lgtm

[calamity, 2014-09-15 01:19:20]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-09-15 01:19:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/557673002/120001

[commit-bot: I haz the power, 2014-09-15 02:15:49]

Committed patchset #3 (id:120001) as 93346fcf6f9c82a94a997589658df9372090dc51

[commit-bot: I haz the power, 2014-09-15 02:17:56]

Patchset 3 (id:??) landed as
https://crrev.com/cc0485133fd3ec2ecb462c8bd1a4a1d5cd34faac
Cr-Commit-Position: refs/heads/master@{#294777}