mus: Keep track of the drag cursor during DnD operations.

mus: Keep track of the drag cursor during DnD operations.

BUG=614037
Committed: https://crrev.com/3e38cbe24d4543a1484c6d3d25626c810de5343c
Cr-Commit-Position: refs/heads/master@{#421621}

[Elliot Glaysher, 2016-09-27 21:45:18]

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

[commit-bot: I haz the power, 2016-09-27 21:45:34]

Dry run: 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-09-27 22:00:02]

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

[commit-bot: I haz the power, 2016-09-27 22:00:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2016-09-27 22:11:17]

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

[commit-bot: I haz the power, 2016-09-27 22:11:38]

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

[Elliot Glaysher, 2016-09-27 23:33:35]

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

[commit-bot: I haz the power, 2016-09-27 23:33:48]

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

[Elliot Glaysher, 2016-09-28 00:10:25]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[commit-bot: I haz the power, 2016-09-28 00:50:35]

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

[commit-bot: I haz the power, 2016-09-28 00:50:35]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-28 15:38:14]

Just naming.

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.cc:38: uint32_t window_operations = 0u;
Please be consistent in the naming. You have OnDragStatusCompleted which takes
uint32_t bitmask, UpdateCursor taking an effect_bitmask, and this which takes
window_operations. AFAICT these are all the same thing. Maybe a typedef (using)
for DropOperations would help clarify what this.

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.cc:177: :
static_cast<int32_t>(ui::mojom::Cursor::COPY);
No link?

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
File services/ui/ws/drag_controller.h (right):

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.h:39: int32_t current_cursor() { return
current_cursor_; }
const

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.h:73: void UpdateCursor(ServerWindow* window,
uint32_t effect_bitmask);
This seems more like SetWindowDropOperations. It may end up updating the cursor
too, but it seems more related to setting the drop operations.

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.h:77: int32_t CursorForWindowOperations(uint32_t
window_operations);
Is there a reason you prefer using int32_t as the type for these rather than
ui::mojom::Cursor?

[Elliot Glaysher, 2016-09-28 18:14:49]

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

[commit-bot: I haz the power, 2016-09-28 18:15:19]

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

[Elliot Glaysher, 2016-09-28 18:15:32]

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
File services/ui/ws/drag_controller.cc (right):

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.cc:38: uint32_t window_operations = 0u;
On 2016/09/28 15:38:13, sky wrote:
> Please be consistent in the naming. You have OnDragStatusCompleted which takes
> uint32_t bitmask, UpdateCursor taking an effect_bitmask, and this which takes
> window_operations. AFAICT these are all the same thing. Maybe a typedef
(using)
> for DropOperations would help clarify what this.

Created a DropEffect and DropEffectBitmask type in the header, changed every
uint32_t which dealt with either to the correct type, and standardized on
bitmask in all functions and here.

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.cc:177: :
static_cast<int32_t>(ui::mojom::Cursor::COPY);
On 2016/09/28 15:38:13, sky wrote:
> No link?

No link. I went through all the cursors and the only NO_DROP and COPY are drag
cursors.

(For chromeos, this is theoretically a place where we could do better and add
cursors for link and move as operations. However, right now those don't exist.)

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
File services/ui/ws/drag_controller.h (right):

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.h:73: void UpdateCursor(ServerWindow* window,
uint32_t effect_bitmask);
On 2016/09/28 15:38:13, sky wrote:
> This seems more like SetWindowDropOperations. It may end up updating the
cursor
> too, but it seems more related to setting the drop operations.

Done.

https://codereview.chromium.org/2376583003/diff/80001/services/ui/ws/drag_con...
services/ui/ws/drag_controller.h:77: int32_t CursorForWindowOperations(uint32_t
window_operations);
On 2016/09/28 15:38:13, sky wrote:
> Is there a reason you prefer using int32_t as the type for these rather than
> ui::mojom::Cursor?

Because the consumer uses int32_t. I can, in a separate patch, go through window
server and replace int32_t with ui::mojom::Cursors (and remove the
static_cast<>s which have become a pattern) if you like.

[commit-bot: I haz the power, 2016-09-28 19:10:44]

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

[commit-bot: I haz the power, 2016-09-28 19:10:45]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-28 20:02:38]

LGTM

https://codereview.chromium.org/2376583003/diff/100001/services/ui/ws/drag_co...
File services/ui/ws/drag_controller.h (right):

https://codereview.chromium.org/2376583003/diff/100001/services/ui/ws/drag_co...
services/ui/ws/drag_controller.h:28: using DropEffect = uint32_t;
Nice! Thanks!

[Elliot Glaysher, 2016-09-28 20:08:51]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-09-28 20:09:09]

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-09-28 20:14:56]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-28 20:17:00]

Description was changed from

==========
mus: Keep track of the drag cursor during DnD operations.

BUG=614037
==========

to

==========
mus: Keep track of the drag cursor during DnD operations.

BUG=614037

Committed: https://crrev.com/3e38cbe24d4543a1484c6d3d25626c810de5343c
Cr-Commit-Position: refs/heads/master@{#421621}
==========

[commit-bot: I haz the power, 2016-09-28 20:17:01]

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