Wires up drags to pointer watcher adapter.

Wires up drags to pointer watcher adapter.

BUG=640410
Committed: https://crrev.com/1dbd4c9de403122e7cdb78d361b5785b85249b0b
Cr-Commit-Position: refs/heads/master@{#416024}

[sammiequon, 2016-08-24 21:26:06]

Patchset #1 (id:1) has been deleted

[sammiequon, 2016-08-24 21:34:24]

Description was changed from

==========
Wires up drags to pointer watcher adapter.

BUG=640410
==========

to

==========
Wires up drags to pointer watcher adapter.

BUG=640410
==========

[sammiequon, 2016-08-24 21:34:24]

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

[sammiequon, 2016-08-24 21:34:48]

sky@ - Please take a look. Thanks!

[sky, 2016-08-24 23:32:30]

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter.h (right):

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter.h:38: bool wants_moves,
Code with lots of boolean parameters gets hard to read and folks end up
introducing temporaries to improve readability. Go with a bitmask to make it
easier to follow.

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter_unittest.cc:135: // Drag: none, no drags for
touch.
Drag should include pointer move.

[sky, 2016-08-24 23:33:02]

https://codereview.chromium.org/2271393002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2271393002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:379:
pointer_watcher_event_router_->AddPointerWatcher(watcher, wants_moves);
Add a NOTIMPLEMENTED in file a bug here for the mus side.

[sammiequon, 2016-08-25 23:11:19]

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter.h (right):

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter.h:38: bool wants_moves,
On 2016/08/24 23:32:30, sky wrote:
> Code with lots of boolean parameters gets hard to read and folks end up
> introducing temporaries to improve readability. Go with a bitmask to make it
> easier to follow.

Done. Though not sure where is the best place to put such an enum.

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/20001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter_unittest.cc:135: // Drag: none, no drags for
touch.
On 2016/08/24 23:32:30, sky wrote:
> Drag should include pointer move.

Done.

https://codereview.chromium.org/2271393002/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2271393002/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:379:
pointer_watcher_event_router_->AddPointerWatcher(watcher, wants_moves);
On 2016/08/24 23:33:01, sky wrote:
> Add a NOTIMPLEMENTED in file a bug here for the mus side.

Done.

[sky, 2016-08-26 15:36:55]

Mostly just naming.

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter.cc (right):

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter.cc:32:
DCHECK(!move_watchers_.HasObserver(watcher));
I think it's ok to assume a watcher isn't added more than once to any list. How
about moving all the DCHECKs outside the conditional.

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter_unittest.cc:92: helper.ExpectCallCount(0, 0, 1,
0, 1, 0);
You can blame me for starting this, but this code is no longer readable. I think
it would be better to supply what is expected as enums. For example, this line
would become
helper.ExpectCalls(MOVE_POINTER | DRAG_POINTER).

https://codereview.chromium.org/2271393002/diff/40001/ash/common/shelf/overfl...
File ash/common/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/2271393002/diff/40001/ash/common/shelf/overfl...
ash/common/shelf/overflow_bubble.cc:22: const views::RequestedEvents events =
views::RequestedEvents::BASIC;
The temporary is useful to understand what a boolean is. Once you have the enum
it's self documenting. In other words change this to:
AddPointerWatcher(this, views::RequestedEvents::BASIC)

This comment applies to all places you're changing, and make sure you do the
rename first.

https://codereview.chromium.org/2271393002/diff/40001/ui/views/pointer_watcher.h
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/2271393002/diff/40001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:23: // to receive. BASIC means they receive only
presses and releases, MOVES means
BASIC also means you get capture events and wheel events.

https://codereview.chromium.org/2271393002/diff/40001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:26: enum class RequestedEvents {
This name is too generic outside of a class, how about PointerWatcherEventTypes.

[sammiequon, 2016-08-26 18:31:20]

Patchset #4 (id:80001) has been deleted

[sammiequon, 2016-08-26 18:34:20]

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter.cc (right):

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter.cc:32:
DCHECK(!move_watchers_.HasObserver(watcher));
On 2016/08/26 15:36:54, sky wrote:
> I think it's ok to assume a watcher isn't added more than once to any list.
How
> about moving all the DCHECKs outside the conditional.

Done.

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/40001/ash/aura/pointer_watche...
ash/aura/pointer_watcher_adapter_unittest.cc:92: helper.ExpectCallCount(0, 0, 1,
0, 1, 0);
On 2016/08/26 15:36:54, sky wrote:
> You can blame me for starting this, but this code is no longer readable. I
think
> it would be better to supply what is expected as enums. For example, this line
> would become
> helper.ExpectCalls(MOVE_POINTER | DRAG_POINTER).

Done.

https://codereview.chromium.org/2271393002/diff/40001/ash/common/shelf/overfl...
File ash/common/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/2271393002/diff/40001/ash/common/shelf/overfl...
ash/common/shelf/overflow_bubble.cc:22: const views::RequestedEvents events =
views::RequestedEvents::BASIC;
On 2016/08/26 15:36:55, sky wrote:
> The temporary is useful to understand what a boolean is. Once you have the
enum
> it's self documenting. In other words change this to:
> AddPointerWatcher(this, views::RequestedEvents::BASIC)
> 
> This comment applies to all places you're changing, and make sure you do the
> rename first.

Done.

https://codereview.chromium.org/2271393002/diff/40001/ui/views/pointer_watcher.h
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/2271393002/diff/40001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:23: // to receive. BASIC means they receive only
presses and releases, MOVES means
On 2016/08/26 15:36:55, sky wrote:
> BASIC also means you get capture events and wheel events.

Done.

https://codereview.chromium.org/2271393002/diff/40001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:26: enum class RequestedEvents {
On 2016/08/26 15:36:55, sky wrote:
> This name is too generic outside of a class, how about
PointerWatcherEventTypes.

Done.

[sky, 2016-08-26 19:58:57]

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter.cc (right):

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:30: // the pair of |watcher| and
|wants_move| and |wants_drags| unique, just
update comment.

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:35: if (events ==
views::PointerWatcherEventTypes::DRAGS) {
Now that all paths are a single line you can nuke the {}

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:62: // The mus version does not send drags.
I think this comment is no longer helpful, you may as well nuke it.

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:114: original_event.type() !=
ui::ET_MOUSE_DRAGGED) {
ET_POINTER_MOVED covers ET_MOUSE_DRAGGED, right? I'm saying you shouldn't need
the second part of this.

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:72: void ExpectCallCount(int
pointer, int capture) {
Can't you combine them all into a single bitmask? Also, I name these
pointer_bitmask and capture_bitmask with a comment indicating what they are.

https://codereview.chromium.org/2271393002/diff/100001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2271393002/diff/100001/ash/common/wm_shell.h#...
ash/common/wm_shell.h:329: // called for pointer move events. If |wants_drags|
is true
Update comment.

https://codereview.chromium.org/2271393002/diff/100001/ui/views/pointer_watch...
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/2271393002/diff/100001/ui/views/pointer_watch...
ui/views/pointer_watcher.h:28: BASIC = 0x01,
This isn't used as a bitmask, right? In other words you shouldn't need explicit
values, right?

[sammiequon, 2016-08-26 21:45:27]

Patchset #5 (id:120001) has been deleted

[sammiequon, 2016-08-26 21:48:04]

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter.cc (right):

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:30: // the pair of |watcher| and
|wants_move| and |wants_drags| unique, just
On 2016/08/26 19:58:57, sky wrote:
> update comment.

Done.

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:35: if (events ==
views::PointerWatcherEventTypes::DRAGS) {
On 2016/08/26 19:58:57, sky wrote:
> Now that all paths are a single line you can nuke the {}

Done.

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:62: // The mus version does not send drags.
On 2016/08/26 19:58:57, sky wrote:
> I think this comment is no longer helpful, you may as well nuke it.

Done.

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter.cc:114: original_event.type() !=
ui::ET_MOUSE_DRAGGED) {
On 2016/08/26 19:58:57, sky wrote:
> ET_POINTER_MOVED covers ET_MOUSE_DRAGGED, right? I'm saying you shouldn't need
> the second part of this.

Done.

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/100001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:72: void ExpectCallCount(int
pointer, int capture) {
On 2016/08/26 19:58:57, sky wrote:
> Can't you combine them all into a single bitmask? Also, I name these
> pointer_bitmask and capture_bitmask with a comment indicating what they are.

Done.

https://codereview.chromium.org/2271393002/diff/100001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2271393002/diff/100001/ash/common/wm_shell.h#...
ash/common/wm_shell.h:329: // called for pointer move events. If |wants_drags|
is true
On 2016/08/26 19:58:57, sky wrote:
> Update comment.

Done.

https://codereview.chromium.org/2271393002/diff/100001/ui/views/pointer_watch...
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/2271393002/diff/100001/ui/views/pointer_watch...
ui/views/pointer_watcher.h:28: BASIC = 0x01,
On 2016/08/26 19:58:57, sky wrote:
> This isn't used as a bitmask, right? In other words you shouldn't need
explicit
> values, right?

Right I thought I'd be using one but forgot to change it. Done.

[sammiequon, 2016-08-29 16:58:01]

Patchset #6 (id:160001) has been deleted

[sky, 2016-08-31 23:27:39]

https://codereview.chromium.org/2271393002/diff/180001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/180001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:68: :
non_move_watcher_(views::PointerWatcherEventTypes::BASIC),
These names are now confusing. It should be basic_watcher, move_watcher and
drag_watcher. The parameters names supplied to ExectCallCount should match that.

https://codereview.chromium.org/2271393002/diff/180001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:78: void ExpectCallCount(int
pointer_bitmask,
I'm confused by what you have here. I would expect each parameter to correspond
to the events received for a particular PointerWatcher configuration. For
example, the first argument would be basic_events_bitmask and consist of a
bitmask of the types expected for a PointerWatcher configured with
views::PointerWatcherEventTypes::BASIC.

What you have as TestPointerCaptureEvents isn't rich enough to capture what the
code should test. It should be:
NONE,
CAPTURE,
WHEEL,
PRESS_ON_RELEASE,
MOVE_OR_DRAG.

https://codereview.chromium.org/2271393002/diff/180001/ui/views/pointer_watch...
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/2271393002/diff/180001/ui/views/pointer_watch...
ui/views/pointer_watcher.h:22: // When pointer watchers are added they need to
specify which events they want
How about:

When a PointerWatcher is added the types of events desired is specified by way
of PointerWatcherEventTypes.

Then move the comments for each type above the enum values, e.g.:
// The PointerWatcher is interested in press, release, capture and mouse wheel.
BASIC,
// The PointerWatcher is interested in BASIC events, as well as move events.
...

[sammiequon, 2016-09-01 00:49:24]

https://codereview.chromium.org/2271393002/diff/180001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/180001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:68: :
non_move_watcher_(views::PointerWatcherEventTypes::BASIC),
On 2016/08/31 23:27:39, sky wrote:
> These names are now confusing. It should be basic_watcher, move_watcher and
> drag_watcher. The parameters names supplied to ExectCallCount should match
that.

Done.

https://codereview.chromium.org/2271393002/diff/180001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:78: void ExpectCallCount(int
pointer_bitmask,
On 2016/08/31 23:27:39, sky wrote:
> I'm confused by what you have here. I would expect each parameter to
correspond
> to the events received for a particular PointerWatcher configuration. For
> example, the first argument would be basic_events_bitmask and consist of a
> bitmask of the types expected for a PointerWatcher configured with
> views::PointerWatcherEventTypes::BASIC.
> 
> What you have as TestPointerCaptureEvents isn't rich enough to capture what
the
> code should test. It should be:
> NONE,
> CAPTURE,
> WHEEL,
> PRESS_ON_RELEASE,
> MOVE_OR_DRAG.

Done.

https://codereview.chromium.org/2271393002/diff/180001/ui/views/pointer_watch...
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/2271393002/diff/180001/ui/views/pointer_watch...
ui/views/pointer_watcher.h:22: // When pointer watchers are added they need to
specify which events they want
On 2016/08/31 23:27:39, sky wrote:
> How about:
> 
> When a PointerWatcher is added the types of events desired is specified by way
> of PointerWatcherEventTypes.
> 
> Then move the comments for each type above the enum values, e.g.:
> // The PointerWatcher is interested in press, release, capture and mouse
wheel.
> BASIC,
> // The PointerWatcher is interested in BASIC events, as well as move events.
> ...

Done.

[sky, 2016-09-01 02:54:14]

https://codereview.chromium.org/2271393002/diff/200001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/200001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:53: else if (event.type() ==
ui::ET_POINTER_MOVED)
Sorry for not realizing this last time. The way you have this now it isn't
possible to differentiate move from drag, which are enabled separately so I
think it's worth tracking separately. By that I mean make this more fine grained
so that you can track DRAG separate from MOVE. I think this is important given
the types are now unique. With what you have we wouldn't be able to detect a
regression.

[sammiequon, 2016-09-01 17:06:48]

https://codereview.chromium.org/2271393002/diff/200001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/200001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:53: else if (event.type() ==
ui::ET_POINTER_MOVED)
On 2016/09/01 02:54:14, sky wrote:
> Sorry for not realizing this last time. The way you have this now it isn't
> possible to differentiate move from drag, which are enabled separately so I
> think it's worth tracking separately. By that I mean make this more fine
grained
> so that you can track DRAG separate from MOVE. I think this is important given
> the types are now unique. With what you have we wouldn't be able to detect a
> regression.

Sorry for all the back and forth. Is this way or adding ET_POINTER_DRAGGED the
correct way?

[sky, 2016-09-01 17:17:04]

Almost. And thanks for cleaning it up. It's much easier to read now.

https://codereview.chromium.org/2271393002/diff/220001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/220001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:62: ui::MouseEvent
mouse_event(event);
You shouldn't need to create a new event here. You can identify a mouse drag by
way of:
      if (pointer_event.flags() &
          (EF_LEFT_MOUSE_BUTTON | EF_MIDDLE_MOUSE_BUTTON |
           EF_RIGHT_MOUSE_BUTTON)) {

[sammiequon, 2016-09-01 17:44:12]

https://codereview.chromium.org/2271393002/diff/220001/ash/aura/pointer_watch...
File ash/aura/pointer_watcher_adapter_unittest.cc (right):

https://codereview.chromium.org/2271393002/diff/220001/ash/aura/pointer_watch...
ash/aura/pointer_watcher_adapter_unittest.cc:62: ui::MouseEvent
mouse_event(event);
On 2016/09/01 17:17:04, sky wrote:
> You shouldn't need to create a new event here. You can identify a mouse drag
by
> way of:
>       if (pointer_event.flags() &
>           (EF_LEFT_MOUSE_BUTTON | EF_MIDDLE_MOUSE_BUTTON |
>            EF_RIGHT_MOUSE_BUTTON)) {

Done.

[sky, 2016-09-01 18:25:59]

Thanks! LGTM

[sammiequon, 2016-09-01 18:29:55]

The CQ bit was checked by sammiequon@chromium.org

[commit-bot: I haz the power, 2016-09-01 18:30:37]

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-01 19:48:32]

Description was changed from

==========
Wires up drags to pointer watcher adapter.

BUG=640410
==========

to

==========
Wires up drags to pointer watcher adapter.

BUG=640410
==========

[commit-bot: I haz the power, 2016-09-01 19:48:34]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2016-09-01 19:50:44]

Description was changed from

==========
Wires up drags to pointer watcher adapter.

BUG=640410
==========

to

==========
Wires up drags to pointer watcher adapter.

BUG=640410

Committed: https://crrev.com/1dbd4c9de403122e7cdb78d361b5785b85249b0b
Cr-Commit-Position: refs/heads/master@{#416024}
==========

[commit-bot: I haz the power, 2016-09-01 19:50:45]

Patchset 9 (id:??) landed as
https://crrev.com/1dbd4c9de403122e7cdb78d361b5785b85249b0b
Cr-Commit-Position: refs/heads/master@{#416024}