Remove ShelfItemDelegate::IsDraggable; check for app list button.

Remove ShelfItemDelegate::IsDraggable; check for app list button.

The only button that shouldn't be draggable is the app list.
(arc deferred items were incorrectly marked non-draggable)
Remove the item delegate function to simplify shelf view code.

Add test support for trying to drag the app list button.
Update and expand the related unit test.

BUG=557406
TEST=Automated; no shelf dragging behavior changes.
R=jamescook@chromium.org

Committed: https://crrev.com/086f91b5d4636ea0680b0db2b5ff2ad67d52926d
Cr-Commit-Position: refs/heads/master@{#438614}

[msw, 2016-12-13 22:11:11]

Description was changed from

==========
Remove ShelfItemDelegate::IsDraggable; check for app list button.

BUG=557406
TEST=Automated; no shelf dragging behavior changes.
R=jamescook@chromium.org
==========

to

==========
Remove ShelfItemDelegate::IsDraggable; check for app list button.

The only button that shouldn't be draggable is the app list.
(arc deferred items were incorrectly marked non-draggable)
Remove the item delegate function to simplify shelf view code.

Add test support for trying to drag the app list button.
Update and expand the related unit test.

BUG=557406
TEST=Automated; no shelf dragging behavior changes.
R=jamescook@chromium.org
==========

[msw, 2016-12-13 22:13:14]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 22:13:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-12-13 22:14:28]

Hey James, please take a look; thanks!
FYI Yury, please note the change for arc deferred items.

[commit-bot: I haz the power, 2016-12-13 23:11:35]

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

[commit-bot: I haz the power, 2016-12-13 23:11:36]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-14 17:41:54]

LGTM with a couple things in the test code. Lemme know if you want me to look
again.

https://codereview.chromium.org/2575613002/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2575613002/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:1172: int first_draggable_item =
model_->GetItemIndexForType(TYPE_APP_LIST) + 1;
Maybe comment or DCHECK that if the shelf only has the app list item then it's
impossible to drag, so first_draggable_item / target_index is always a valid
item?

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:443: views::View*
SimulateViewPressed(ShelfView::Pointer pointer, int view_index) {
Is |pointer| always ShelfView::MOUSE?

If so, it shouldn't need the parameter.

If not, it seems weird that this uses ET_MOUSE_PRESSED.

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:452: // Similar to SimulateViewPressed, but the
index must not be for the app list,
Can it DCHECK that button_index is not the app_list index?

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:1200: ASSERT_NO_FATAL_FAILURE(DragAndVerify(5,
0, shelf_view_, id_map));
Thanks for documenting all this.

https://codereview.chromium.org/2575613002/diff/1/ash/test/shelf_view_test_api.h
File ash/test/shelf_view_test_api.h (right):

https://codereview.chromium.org/2575613002/diff/1/ash/test/shelf_view_test_ap...
ash/test/shelf_view_test_api.h:46: // Retrieve the button at |index|, doesn't
support the app list button.
nit: add // because the app list button is not a ShelfButton.

[msw, 2016-12-14 19:46:50]

https://codereview.chromium.org/2575613002/diff/1/ash/common/shelf/shelf_view.cc
File ash/common/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2575613002/diff/1/ash/common/shelf/shelf_view...
ash/common/shelf/shelf_view.cc:1172: int first_draggable_item =
model_->GetItemIndexForType(TYPE_APP_LIST) + 1;
On 2016/12/14 17:41:54, James Cook wrote:
> Maybe comment or DCHECK that if the shelf only has the app list item then it's
> impossible to drag, so first_draggable_item / target_index is always a valid
> item?

Added a comment and a couple DCHECKs. The top of this function also DCHECKs
dragging() and drag_view_, which is never set as the app list button itself
(drag_view_ is a ShelfButton and the app list button oddly isn't), and so if
drag_view_ is set, it must be some other valid draggable button (which comes
after the app list button, which is always first), so this index must be 1 and
must be valid. Really, I could just set this to 1, and DCHECK my assumptions,
but I'll leave the variable assignment as-is for now.

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:443: views::View*
SimulateViewPressed(ShelfView::Pointer pointer, int view_index) {
On 2016/12/14 17:41:54, James Cook wrote:
> Is |pointer| always ShelfView::MOUSE?
> 
> If so, it shouldn't need the parameter.
> 
> If not, it seems weird that this uses ET_MOUSE_PRESSED.

ShelfView::TOUCH is used here via SimulateDrag, and this has worked as-is with
that. Your concern is valid, but out of scope of this CL (I would have to change
all the MouseEvents), I'll leave this alone for now if that's okay with you.

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:452: // Similar to SimulateViewPressed, but the
index must not be for the app list,
On 2016/12/14 17:41:54, James Cook wrote:
> Can it DCHECK that button_index is not the app_list index?

Done.

https://codereview.chromium.org/2575613002/diff/1/ash/shelf/shelf_view_unitte...
ash/shelf/shelf_view_unittest.cc:1200: ASSERT_NO_FATAL_FAILURE(DragAndVerify(5,
0, shelf_view_, id_map));
On 2016/12/14 17:41:54, James Cook wrote:
> Thanks for documenting all this.

Acknowledged.

https://codereview.chromium.org/2575613002/diff/1/ash/test/shelf_view_test_api.h
File ash/test/shelf_view_test_api.h (right):

https://codereview.chromium.org/2575613002/diff/1/ash/test/shelf_view_test_ap...
ash/test/shelf_view_test_api.h:46: // Retrieve the button at |index|, doesn't
support the app list button.
On 2016/12/14 17:41:54, James Cook wrote:
> nit: add // because the app list button is not a ShelfButton.

Done (but hopefully I'll change that in the future).

[msw, 2016-12-14 19:46:53]

The CQ bit was checked by msw@chromium.org

[msw, 2016-12-14 19:46:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2575613002/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-12-14 19:47:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 20:40:46]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481744813944280,
"parent_rev": "24b187b928058ff35ccd994905fa99d4263ade38", "commit_rev":
"02ddf9ce045a0b44dc095d8562e6684388f8ad86"}

[commit-bot: I haz the power, 2016-12-14 20:41:19]

Description was changed from

==========
Remove ShelfItemDelegate::IsDraggable; check for app list button.

The only button that shouldn't be draggable is the app list.
(arc deferred items were incorrectly marked non-draggable)
Remove the item delegate function to simplify shelf view code.

Add test support for trying to drag the app list button.
Update and expand the related unit test.

BUG=557406
TEST=Automated; no shelf dragging behavior changes.
R=jamescook@chromium.org
==========

to

==========
Remove ShelfItemDelegate::IsDraggable; check for app list button.

The only button that shouldn't be draggable is the app list.
(arc deferred items were incorrectly marked non-draggable)
Remove the item delegate function to simplify shelf view code.

Add test support for trying to drag the app list button.
Update and expand the related unit test.

BUG=557406
TEST=Automated; no shelf dragging behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2575613002
==========

[commit-bot: I haz the power, 2016-12-14 20:41:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-14 20:42:56]

Description was changed from

==========
Remove ShelfItemDelegate::IsDraggable; check for app list button.

The only button that shouldn't be draggable is the app list.
(arc deferred items were incorrectly marked non-draggable)
Remove the item delegate function to simplify shelf view code.

Add test support for trying to drag the app list button.
Update and expand the related unit test.

BUG=557406
TEST=Automated; no shelf dragging behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2575613002
==========

to

==========
Remove ShelfItemDelegate::IsDraggable; check for app list button.

The only button that shouldn't be draggable is the app list.
(arc deferred items were incorrectly marked non-draggable)
Remove the item delegate function to simplify shelf view code.

Add test support for trying to drag the app list button.
Update and expand the related unit test.

BUG=557406
TEST=Automated; no shelf dragging behavior changes.
R=jamescook@chromium.org

Committed: https://crrev.com/086f91b5d4636ea0680b0db2b5ff2ad67d52926d
Cr-Commit-Position: refs/heads/master@{#438614}
==========

[commit-bot: I haz the power, 2016-12-14 20:42:57]

Patchset 2 (id:??) landed as
https://crrev.com/086f91b5d4636ea0680b0db2b5ff2ad67d52926d
Cr-Commit-Position: refs/heads/master@{#438614}