Updates PointerEventRouter to handle switching move type

Updates PointerEventRouter to handle switching move type

As part of this I'm removing the id that was added when the api was
with eventmatchers. I can't think of a good way to make the api work
with ids that won't involve raciness. In particular as switching
between move and non-move means changing the event id it's possible
that some events get dropped. This worries me.

Without the ids it means a client might process an event
they don't really care about, but PointerWatcherEventRouter ensures
the right events are routed. For example, lets say there was an in
flight match for a move, but the client switches to not wanting
moves. WindowTreeClient will call OnPointerEventObserver(), but
PointerEventRouter will ensure the event only goes to the
pointerwatchers that want the move.

BUG=627146
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/e37541e6e742816c31c17f993a5842d619bba48d
Cr-Commit-Position: refs/heads/master@{#413182}

[sky, 2016-08-17 23:27:30]

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

[commit-bot: I haz the power, 2016-08-17 23:27:55]

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-08-17 23:41:25]

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

[commit-bot: I haz the power, 2016-08-17 23:41:26]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-17 23:44:40]

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

[commit-bot: I haz the power, 2016-08-17 23:45:11]

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-08-18 01:47:59]

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

[commit-bot: I haz the power, 2016-08-18 01:48:00]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-18 22:06:07]

Description was changed from

==========
Updates PointerEventRouter to handle switching move type

As part of this I'm removing the id that was added when the api was
with eventmatchers. I can't think of a good way to make the api work
with ids that won't involve raciness. In particular as switching
between move and non-move means changing the event id it's possible
that some events get dropped. This worries me.

Without the ids it means a client might process an event
they don't really care about, but PointerWatcherEventRouter ensures
the right events are routed. For example, lets say there was an in
flight match for a move, but the client switches to not wanting
moves. WindowTreeClient will call OnPointerEventObserver(), but
PointerEventRouter will ensure the event only goes to the
pointerwatchers that want the move.

BUG=627146
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Updates PointerEventRouter to handle switching move type

As part of this I'm removing the id that was added when the api was
with eventmatchers. I can't think of a good way to make the api work
with ids that won't involve raciness. In particular as switching
between move and non-move means changing the event id it's possible
that some events get dropped. This worries me.

Without the ids it means a client might process an event
they don't really care about, but PointerWatcherEventRouter ensures
the right events are routed. For example, lets say there was an in
flight match for a move, but the client switches to not wanting
moves. WindowTreeClient will call OnPointerEventObserver(), but
PointerEventRouter will ensure the event only goes to the
pointerwatchers that want the move.

BUG=627146
TEST=covered by tests
R=jamescook@chromium.org
==========

[sky, 2016-08-18 22:06:07]

sky@chromium.org changed reviewers:
+ msw@chromium.org
- jamescook@chromium.org

[sky, 2016-08-18 22:06:25]

Mike, any chance you could look at this as James is busy with gardening?

[msw, 2016-08-18 22:45:23]

lgtm with nits and qs.

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_tree_client_unittest.cc (left):

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_tree_client_unittest.cc:537:
TEST_F(WindowTreeClientTest, PointerWatcherReplaced) {
Is there a way to keep this test, perhaps at a different level?
(does something similar already exist at window_tree_unittest.cc?)

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/window_tree_client.cc:985: // Non-zero pointer_watcher_id
means it matched a pointer watcher on the
nit: update comment

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:92: // a system-wide
performance/ battery penalty for listening to moves.
nit: no space after slash

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:380: // events until the event
is ack'ed.
optional nit: clarify if pointer watcher events should be acked?

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:386: // Called when a pointer
event that would not normally be targeted at this
aside q: Could we fold this into OnWindowInputEvent? (separately, if at all)

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router.cc (right):

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router.cc:45: // Pointer watchers cannot be
added multiple times.
Should this check both lists? (ditto below)

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router_unittest.cc (right):

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router_unittest.cc:91: // Remove moves.
optional nit: test the transition from MOVE_EVENTS to NON_MOVE_EVENTS?

[sky, 2016-08-18 23:17:07]

The CQ bit was checked by sky@chromium.org

[sky, 2016-08-18 23:17:07]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2248263003/#ps80001
(title: "feedback")

[sky, 2016-08-18 23:17:15]

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_tree_client_unittest.cc (left):

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_tree_client_unittest.cc:537:
TEST_F(WindowTreeClientTest, PointerWatcherReplaced) {
On 2016/08/18 22:45:23, msw wrote:
> Is there a way to keep this test, perhaps at a different level?
> (does something similar already exist at window_tree_unittest.cc?)

Previously this case was interesting because of the complexity and remembering
of ids. Now that ids are gone this case isn't particular interesting that it
warrants a test.

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/window_tree_client.cc:985: // Non-zero pointer_watcher_id
means it matched a pointer watcher on the
On 2016/08/18 22:45:23, msw wrote:
> nit: update comment

I just removed the comment as the code isn't particularly tricky at this point
that it warrants a comment.

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:92: // a system-wide
performance/ battery penalty for listening to moves.
On 2016/08/18 22:45:23, msw wrote:
> nit: no space after slash

Done.

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:380: // events until the event
is ack'ed.
On 2016/08/18 22:45:23, msw wrote:
> optional nit: clarify if pointer watcher events should be acked?

Done.

https://codereview.chromium.org/2248263003/diff/40001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:386: // Called when a pointer
event that would not normally be targeted at this
On 2016/08/18 22:45:23, msw wrote:
> aside q: Could we fold this into OnWindowInputEvent? (separately, if at all)

We certainly could, but in some ways it's nice to have the two as one requires a
response and one doesn't. I'm going to keep it like this for now.

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router.cc (right):

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router.cc:45: // Pointer watchers cannot be
added multiple times.
On 2016/08/18 22:45:23, msw wrote:
> Should this check both lists? (ditto below)

Done.

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router_unittest.cc (right):

https://codereview.chromium.org/2248263003/diff/40001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router_unittest.cc:91: // Remove moves.
On 2016/08/18 22:45:23, msw wrote:
> optional nit: test the transition from MOVE_EVENTS to NON_MOVE_EVENTS?

Done.

[sky, 2016-08-18 23:17:36]

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

[commit-bot: I haz the power, 2016-08-18 23:18:07]

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

[sky, 2016-08-18 23:18:12]

+tsepez for window_tree.mojom

[commit-bot: I haz the power, 2016-08-18 23:25:57]

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

[commit-bot: I haz the power, 2016-08-18 23:25:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sky, 2016-08-19 15:11:38]

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

[commit-bot: I haz the power, 2016-08-19 15:12:03]

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-08-19 17:09:21]

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

[commit-bot: I haz the power, 2016-08-19 17:09:22]

Dry run: Exceeded global retry quota

[Tom Sepez, 2016-08-19 17:38:39]

mojom LGTM

[sky, 2016-08-19 17:44:46]

The CQ bit was checked by sky@chromium.org

[sky, 2016-08-19 17:44:46]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2248263003/#ps120001
(title: "merge again")

[commit-bot: I haz the power, 2016-08-19 17:45:40]

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-08-19 17:51:17]

Description was changed from

==========
Updates PointerEventRouter to handle switching move type

As part of this I'm removing the id that was added when the api was
with eventmatchers. I can't think of a good way to make the api work
with ids that won't involve raciness. In particular as switching
between move and non-move means changing the event id it's possible
that some events get dropped. This worries me.

Without the ids it means a client might process an event
they don't really care about, but PointerWatcherEventRouter ensures
the right events are routed. For example, lets say there was an in
flight match for a move, but the client switches to not wanting
moves. WindowTreeClient will call OnPointerEventObserver(), but
PointerEventRouter will ensure the event only goes to the
pointerwatchers that want the move.

BUG=627146
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Updates PointerEventRouter to handle switching move type

As part of this I'm removing the id that was added when the api was
with eventmatchers. I can't think of a good way to make the api work
with ids that won't involve raciness. In particular as switching
between move and non-move means changing the event id it's possible
that some events get dropped. This worries me.

Without the ids it means a client might process an event
they don't really care about, but PointerWatcherEventRouter ensures
the right events are routed. For example, lets say there was an in
flight match for a move, but the client switches to not wanting
moves. WindowTreeClient will call OnPointerEventObserver(), but
PointerEventRouter will ensure the event only goes to the
pointerwatchers that want the move.

BUG=627146
TEST=covered by tests
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-08-19 17:51:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-19 17:58:12]

Description was changed from

==========
Updates PointerEventRouter to handle switching move type

As part of this I'm removing the id that was added when the api was
with eventmatchers. I can't think of a good way to make the api work
with ids that won't involve raciness. In particular as switching
between move and non-move means changing the event id it's possible
that some events get dropped. This worries me.

Without the ids it means a client might process an event
they don't really care about, but PointerWatcherEventRouter ensures
the right events are routed. For example, lets say there was an in
flight match for a move, but the client switches to not wanting
moves. WindowTreeClient will call OnPointerEventObserver(), but
PointerEventRouter will ensure the event only goes to the
pointerwatchers that want the move.

BUG=627146
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Updates PointerEventRouter to handle switching move type

As part of this I'm removing the id that was added when the api was
with eventmatchers. I can't think of a good way to make the api work
with ids that won't involve raciness. In particular as switching
between move and non-move means changing the event id it's possible
that some events get dropped. This worries me.

Without the ids it means a client might process an event
they don't really care about, but PointerWatcherEventRouter ensures
the right events are routed. For example, lets say there was an in
flight match for a move, but the client switches to not wanting
moves. WindowTreeClient will call OnPointerEventObserver(), but
PointerEventRouter will ensure the event only goes to the
pointerwatchers that want the move.

BUG=627146
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/e37541e6e742816c31c17f993a5842d619bba48d
Cr-Commit-Position: refs/heads/master@{#413182}
==========

[commit-bot: I haz the power, 2016-08-19 17:58:13]

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