mus: Change PointerWatcher to observe all pointer events, with moves optional.

mus: Change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and
discussions from https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests,
     mus_ws_unittests

Committed: https://crrev.com/13661fe1ec3d5083450e45a25401b1ec93a61d85
Cr-Commit-Position: refs/heads/master@{#410734}

[riajiang, 2016-07-26 19:32:33]

Description was changed from

==========
BUG=628663
TEST=views_mus_unittests
==========

to

==========
mus: change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and discussions from
https://codereview.chromium.org/2163453002/.

BUG=628663
TEST=views_mus_unittests, mus_public_unittests
==========

[riajiang, 2016-07-26 19:32:33]

riajiang@chromium.org changed reviewers:
+ jamescook@chromium.org, sadrul@chromium.org, sky@chromium.org

[riajiang, 2016-07-26 21:45:32]

Description was changed from

==========
mus: change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and discussions from
https://codereview.chromium.org/2163453002/.

BUG=628663
TEST=views_mus_unittests, mus_public_unittests
==========

to

==========
mus: change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and discussions from
https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests
==========

[James Cook, 2016-07-27 01:54:02]

I'm biased, but I like it. :-)

https://codereview.chromium.org/2183163002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_event_filter.cc:42:
ProcessPressedEvent(location_in_screen, target);
(BTW, this should fix my bug with the system tray not closing.)

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_tree_client_unittest.cc:482: new
ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
Change all these test events to PointerEvents, since that's what the pointer
watcher actually uses.

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

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:90: //
I would add a note (or revise the above) to say that clients should prefer
|want_moves| to be false, as there's a system-wide performance/battery penalty
for listening to moves.

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:384:
OnEventObserved(ui.mojom.Event event, uint32 pointer_watcher_id);
I think this should be renamed to OnPointerWatcherEvent(), since there's no
SetEventObserver anymore. (Sorry to ask for another rename. :-)

https://codereview.chromium.org/2183163002/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1002: if (!pointer_watcher_want_moves_ &&
event.type() == ui::ET_POINTER_MOVED)
For now, let's also limit POINTER_ENTER and POINTER_EXIT to the clients that
want moves. Ash doesn't need them yet.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.cc:189: pointer_event = new
ui::PointerEvent(*event.AsTouchEvent());
Does this conversion have to happen, or could the caller of OnEventObserved()
pass a PointerEvent? Maybe the signature should change to take PointerEvent
instead of ui::Event.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection_unittest.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection_unittest.cc:28: void set_want_moves(bool
want_moves) { want_moves_ = want_moves; }
I wouldn't explicitly set_want_moves(), just rely on whether you added the
pointerwatcher with want_moves true.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection_unittest.cc:38: void
OnPointerWatcherEvent(const ui::PointerEvent& event,
The tests might be easier to reason about if this just cached a copy of the last
event and let tests compare against it.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection_unittest.cc:138:
TEST_F(WindowManagerConnectionTest, PointerWatcherMove) {
This test could probably be simplified to just check that it gets POINTER_MOVE /
ENTER / EXIT and not deal with touch/mouse events at all.

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

https://codereview.chromium.org/2183163002/diff/20001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:35: virtual void OnPointerWatcherEvent(const
ui::PointerEvent& event,
Thanks for renaming this!

[riajiang, 2016-07-27 21:43:57]

Description was changed from

==========
mus: change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and discussions from
https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests
==========

to

==========
mus: change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and discussions from
https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests, mus_ws_unittests
==========

[riajiang, 2016-07-27 22:39:16]

https://codereview.chromium.org/2183163002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_event_filter.cc:42:
ProcessPressedEvent(location_in_screen, target);
On 2016/07/27 01:54:01, James Cook wrote:
> (BTW, this should fix my bug with the system tray not closing.)

Tested and added that to the bug list.

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_tree_client_unittest.cc:482: new
ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
On 2016/07/27 01:54:01, James Cook wrote:
> Change all these test events to PointerEvents, since that's what the pointer
> watcher actually uses.

Done.

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

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:90: //
On 2016/07/27 01:54:01, James Cook wrote:
> I would add a note (or revise the above) to say that clients should prefer
> |want_moves| to be false, as there's a system-wide performance/battery penalty
> for listening to moves.

Done.

https://codereview.chromium.org/2183163002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:384:
OnEventObserved(ui.mojom.Event event, uint32 pointer_watcher_id);
On 2016/07/27 01:54:01, James Cook wrote:
> I think this should be renamed to OnPointerWatcherEvent(), since there's no
> SetEventObserver anymore. (Sorry to ask for another rename. :-)

No worries, done :)

https://codereview.chromium.org/2183163002/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1002: if (!pointer_watcher_want_moves_ &&
event.type() == ui::ET_POINTER_MOVED)
On 2016/07/27 01:54:01, James Cook wrote:
> For now, let's also limit POINTER_ENTER and POINTER_EXIT to the clients that
> want moves. Ash doesn't need them yet.

Done.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.cc:189: pointer_event = new
ui::PointerEvent(*event.AsTouchEvent());
On 2016/07/27 01:54:01, James Cook wrote:
> Does this conversion have to happen, or could the caller of OnEventObserved()
> pass a PointerEvent? Maybe the signature should change to take PointerEvent
> instead of ui::Event.

Changed to take a PointerEvent. The conversion now happens in
WindowTreeClient::OnWindowInputEvent and
WindowTreeClient::OnPointerWatcherEvent.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection_unittest.cc (right):

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection_unittest.cc:28: void set_want_moves(bool
want_moves) { want_moves_ = want_moves; }
On 2016/07/27 01:54:02, James Cook wrote:
> I wouldn't explicitly set_want_moves(), just rely on whether you added the
> pointerwatcher with want_moves true.

Done.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection_unittest.cc:38: void
OnPointerWatcherEvent(const ui::PointerEvent& event,
On 2016/07/27 01:54:01, James Cook wrote:
> The tests might be easier to reason about if this just cached a copy of the
last
> event and let tests compare against it.

Done.

https://codereview.chromium.org/2183163002/diff/20001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection_unittest.cc:138:
TEST_F(WindowManagerConnectionTest, PointerWatcherMove) {
On 2016/07/27 01:54:01, James Cook wrote:
> This test could probably be simplified to just check that it gets POINTER_MOVE
/
> ENTER / EXIT and not deal with touch/mouse events at all.

Done.

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

https://codereview.chromium.org/2183163002/diff/20001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:35: virtual void OnPointerWatcherEvent(const
ui::PointerEvent& event,
On 2016/07/27 01:54:02, James Cook wrote:
> Thanks for renaming this!

You are welcome!

[James Cook, 2016-07-27 23:21:22]

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

https://codereview.chromium.org/2183163002/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/lib/window_tree_client.cc:1262: ui::PointerEvent*
WindowTreeClient::ConvertEventToPointerEvent(
This function leaks memory for every event matched by a PointerWatcher.

One approach to fix this would be to make this function
OnPointerWatcherEventImpl(), and call delegate_ here, similar to how
OnWindowInputEvent() handles its event type conversions on line 999.

There are probably other ways to fix it, but it would be nice to avoid heap
allocations if possible.

https://codereview.chromium.org/2183163002/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/2183163002/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree_unittest.cc:320: // Create an observer for all events
excluding move events.
nit: "an observer" -> "a watcher" in comments throughout these tests

[nabila mehjabin, 2016-07-29 20:22:22]

On 2016/07/27 23:21:22, James Cook (out until 8-1) wrote:
>
https://codereview.chromium.org/2183163002/diff/40001/services/ui/public/cpp/...
> File services/ui/public/cpp/lib/window_tree_client.cc (right):
> 
>
https://codereview.chromium.org/2183163002/diff/40001/services/ui/public/cpp/...
> services/ui/public/cpp/lib/window_tree_client.cc:1262: ui::PointerEvent*
> WindowTreeClient::ConvertEventToPointerEvent(
> This function leaks memory for every event matched by a PointerWatcher.
> 
> One approach to fix this would be to make this function
> OnPointerWatcherEventImpl(), and call delegate_ here, similar to how
> OnWindowInputEvent() handles its event type conversions on line 999.
> 
> There are probably other ways to fix it, but it would be nice to avoid heap
> allocations if possible.
> 
>
https://codereview.chromium.org/2183163002/diff/40001/services/ui/ws/window_t...
> File services/ui/ws/window_tree_unittest.cc (right):
> 
>
https://codereview.chromium.org/2183163002/diff/40001/services/ui/ws/window_t...
> services/ui/ws/window_tree_unittest.cc:320: // Create an observer for all
events
> excluding move events.
> nit: "an observer" -> "a watcher" in comments throughout these tests

Restart for all service DNS address could on media network

[sadrul, 2016-07-29 20:26:10]

I was thinking we could preserve the old code for installing EventObservers
based on EventMatcher, but I think it does make sense to remove now. We can
always add it back later if/when we need it.

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/cpp/...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/cpp/...
services/ui/public/cpp/lib/window_tree_client.cc:1271: }
It doesn't look like we should get a Mouse or a Touch event here. Can you
DCHECK() instead that we are not getting those?

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

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:385:
OnPointerWatcherEvent(ui.mojom.Event event, uint32 pointer_watcher_id);
OnPointerEventObserved

https://codereview.chromium.org/2183163002/diff/60001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/60001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1006: return true;
return pointer_watcher_want_moves_ ||
   (event.type() == POINTER_DOWN || event.type() == POINTER_UP)

https://codereview.chromium.org/2183163002/diff/60001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2183163002/diff/60001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.cc:177: return;
This should now be a DCHECK()

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

https://codereview.chromium.org/2183163002/diff/60001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:34: // them.
Add the note in the comment above instead.

https://codereview.chromium.org/2183163002/diff/60001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:37: Widget* target) = 0;
Let's call this OnPointerEvent

[James Cook, 2016-08-01 16:43:55]

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

https://codereview.chromium.org/2183163002/diff/60001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:37: Widget* target) = 0;
On 2016/07/29 20:26:10, sadrul wrote:
> Let's call this OnPointerEvent

Sadrul, are you sure? That makes it look a lot like EventHandler methods
OnMouseEvent, OnTouchEvent, etc. and there's currently a class 
(ShelfTooltipManager) that is both an EventHandler and a PointerWatcher. I
prefer what it is now, but if you feel strongly I'm OK with OnPointerEvent.

[sadrul, 2016-08-02 16:13:51]

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/cpp/...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/cpp/...
services/ui/public/cpp/lib/window_tree_client.cc:1271: }
On 2016/07/29 20:26:10, sadrul wrote:
> It doesn't look like we should get a Mouse or a Touch event here. Can you
> DCHECK() instead that we are not getting those?

if (event.IsPointerEvent()) {
} else {
  DCHECK(!mouse && !touch);
}

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

https://codereview.chromium.org/2183163002/diff/60001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:37: Widget* target) = 0;
On 2016/08/01 16:43:54, James Cook wrote:
> On 2016/07/29 20:26:10, sadrul wrote:
> > Let's call this OnPointerEvent
> 
> Sadrul, are you sure? That makes it look a lot like EventHandler methods
> OnMouseEvent, OnTouchEvent, etc. and there's currently a class 
> (ShelfTooltipManager) that is both an EventHandler and a PointerWatcher. I
> prefer what it is now, but if you feel strongly I'm OK with OnPointerEvent.

Hm. I agree. Calling this OnPointerEvent could be a bit confusing. I am not a
huge fan of the current name though. How about OnPointerEventObserved()?

[James Cook, 2016-08-02 16:18:41]

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

https://codereview.chromium.org/2183163002/diff/60001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:37: Widget* target) = 0;
On 2016/08/02 16:13:51, sadrul wrote:
> On 2016/08/01 16:43:54, James Cook wrote:
> > On 2016/07/29 20:26:10, sadrul wrote:
> > > Let's call this OnPointerEvent
> > 
> > Sadrul, are you sure? That makes it look a lot like EventHandler methods
> > OnMouseEvent, OnTouchEvent, etc. and there's currently a class 
> > (ShelfTooltipManager) that is both an EventHandler and a PointerWatcher. I
> > prefer what it is now, but if you feel strongly I'm OK with OnPointerEvent.
> 
> Hm. I agree. Calling this OnPointerEvent could be a bit confusing. I am not a
> huge fan of the current name though. How about OnPointerEventObserved()?

sgtm

[riajiang, 2016-08-02 19:59:53]

Please take another look, thanks!

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

https://codereview.chromium.org/2183163002/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/lib/window_tree_client.cc:1262: ui::PointerEvent*
WindowTreeClient::ConvertEventToPointerEvent(
On 2016/07/27 23:21:21, James Cook (out until 8-1) wrote:
> This function leaks memory for every event matched by a PointerWatcher.
> 
> One approach to fix this would be to make this function
> OnPointerWatcherEventImpl(), and call delegate_ here, similar to how
> OnWindowInputEvent() handles its event type conversions on line 999.
> 
> There are probably other ways to fix it, but it would be nice to avoid heap
> allocations if possible.

Moved it back since it's back to a one-liner.

https://codereview.chromium.org/2183163002/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/2183163002/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree_unittest.cc:320: // Create an observer for all events
excluding move events.
On 2016/07/27 23:21:22, James Cook (out until 8-1) wrote:
> nit: "an observer" -> "a watcher" in comments throughout these tests

Done.

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/cpp/...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/cpp/...
services/ui/public/cpp/lib/window_tree_client.cc:1271: }
On 2016/08/02 16:13:51, sadrul wrote:
> On 2016/07/29 20:26:10, sadrul wrote:
> > It doesn't look like we should get a Mouse or a Touch event here. Can you
> > DCHECK() instead that we are not getting those?
> 
> if (event.IsPointerEvent()) {
> } else {
>   DCHECK(!mouse && !touch);
> }

Added DCHECK and moved this back to OnWindowInputEvent and OnPointerWatcherEvent
since it's back to a one-liner?

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

https://codereview.chromium.org/2183163002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:385:
OnPointerWatcherEvent(ui.mojom.Event event, uint32 pointer_watcher_id);
On 2016/07/29 20:26:10, sadrul wrote:
> OnPointerEventObserved

Done.

https://codereview.chromium.org/2183163002/diff/60001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/60001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1006: return true;
On 2016/07/29 20:26:10, sadrul wrote:
> return pointer_watcher_want_moves_ ||
>    (event.type() == POINTER_DOWN || event.type() == POINTER_UP)

Done.

https://codereview.chromium.org/2183163002/diff/60001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2183163002/diff/60001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.cc:177: return;
On 2016/07/29 20:26:10, sadrul wrote:
> This should now be a DCHECK()

Done.

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

https://codereview.chromium.org/2183163002/diff/60001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:34: // them.
On 2016/07/29 20:26:10, sadrul wrote:
> Add the note in the comment above instead.

Done.

https://codereview.chromium.org/2183163002/diff/60001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:37: Widget* target) = 0;
On 2016/08/02 16:18:41, James Cook wrote:
> On 2016/08/02 16:13:51, sadrul wrote:
> > On 2016/08/01 16:43:54, James Cook wrote:
> > > On 2016/07/29 20:26:10, sadrul wrote:
> > > > Let's call this OnPointerEvent
> > > 
> > > Sadrul, are you sure? That makes it look a lot like EventHandler methods
> > > OnMouseEvent, OnTouchEvent, etc. and there's currently a class 
> > > (ShelfTooltipManager) that is both an EventHandler and a PointerWatcher. I
> > > prefer what it is now, but if you feel strongly I'm OK with
OnPointerEvent.
> > 
> > Hm. I agree. Calling this OnPointerEvent could be a bit confusing. I am not
a
> > huge fan of the current name though. How about OnPointerEventObserved()?
> 
> sgtm

Done.

[James Cook, 2016-08-02 21:19:51]

https://codereview.chromium.org/2183163002/diff/100001/ash/pointer_watcher_de...
File ash/pointer_watcher_delegate_aura.cc (right):

https://codereview.chromium.org/2183163002/diff/100001/ash/pointer_watcher_de...
ash/pointer_watcher_delegate_aura.cc:38: if (event->type() ==
ui::ET_MOUSE_MOVED)
Now that I think about it more, these tests should probably be to allow only
ET_MOUSE_PRESSED and ET_MOUSE_RELEASED, rather than just blocking moves. That
better matches mus. Something like:

if (type != PRESSED && type != RELEASED)
  return;

https://codereview.chromium.org/2183163002/diff/100001/ash/pointer_watcher_de...
ash/pointer_watcher_delegate_aura.cc:49: if (event->type() ==
ui::ET_TOUCH_MOVED)
ditto

https://codereview.chromium.org/2183163002/diff/100001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/100001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1002: if (!pointer_watcher_want_moves_ &&
(event.type() == ui::ET_POINTER_MOVED ||
I think you can skip this if() now. The statement below is basically what we
want -- either pass everything or just pass DOWN/UP. I don't think
ET_POINTER_CANCELLED matters either way.

[riajiang, 2016-08-02 22:01:41]

https://codereview.chromium.org/2183163002/diff/100001/ash/pointer_watcher_de...
File ash/pointer_watcher_delegate_aura.cc (right):

https://codereview.chromium.org/2183163002/diff/100001/ash/pointer_watcher_de...
ash/pointer_watcher_delegate_aura.cc:38: if (event->type() ==
ui::ET_MOUSE_MOVED)
On 2016/08/02 21:19:50, James Cook wrote:
> Now that I think about it more, these tests should probably be to allow only
> ET_MOUSE_PRESSED and ET_MOUSE_RELEASED, rather than just blocking moves. That
> better matches mus. Something like:
> 
> if (type != PRESSED && type != RELEASED)
>   return;

Done.

https://codereview.chromium.org/2183163002/diff/100001/ash/pointer_watcher_de...
ash/pointer_watcher_delegate_aura.cc:49: if (event->type() ==
ui::ET_TOUCH_MOVED)
On 2016/08/02 21:19:50, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2183163002/diff/100001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/100001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1002: if (!pointer_watcher_want_moves_ &&
(event.type() == ui::ET_POINTER_MOVED ||
On 2016/08/02 21:19:51, James Cook wrote:
> I think you can skip this if() now. The statement below is basically what we
> want -- either pass everything or just pass DOWN/UP. I don't think
> ET_POINTER_CANCELLED matters either way.

True, done.

[James Cook, 2016-08-02 23:20:36]

LGTM with nit

https://codereview.chromium.org/2183163002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1002: return pointer_watcher_want_moves_ ||
(event.type() == ui::ET_POINTER_DOWN ||
nit: () not needed

[sadrul, 2016-08-03 16:12:44]

https://codereview.chromium.org/2183163002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:977:
DCHECK(event->IsPointerEvent());
Why the DCHECK here? We can receive ui::KeyEvent here, for example.

https://codereview.chromium.org/2183163002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:983: pointer_watcher_id ==
pointer_watcher_id_) {
&& event->IsPointerEvent()

https://codereview.chromium.org/2183163002/diff/120001/ui/views/mus/window_ma...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2183163002/diff/120001/ui/views/mus/window_ma...
ui/views/mus/window_manager_connection.cc:176: DCHECK(event.IsLocatedEvent());
You would DCHECK if |event| was a ui::Event, but since |event| is a PointerEvent
type, you don't need this DCHECK

[riajiang, 2016-08-03 17:54:23]

https://codereview.chromium.org/2183163002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:977:
DCHECK(event->IsPointerEvent());
On 2016/08/03 16:12:44, sadrul wrote:
> Why the DCHECK here? We can receive ui::KeyEvent here, for example.

Removed.

https://codereview.chromium.org/2183163002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:983: pointer_watcher_id ==
pointer_watcher_id_) {
On 2016/08/03 16:12:44, sadrul wrote:
> && event->IsPointerEvent()

Done.

https://codereview.chromium.org/2183163002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1002: return pointer_watcher_want_moves_ ||
(event.type() == ui::ET_POINTER_DOWN ||
On 2016/08/02 23:20:36, James Cook (OOO until 8-5) wrote:
> nit: () not needed

Done.

https://codereview.chromium.org/2183163002/diff/120001/ui/views/mus/window_ma...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/2183163002/diff/120001/ui/views/mus/window_ma...
ui/views/mus/window_manager_connection.cc:176: DCHECK(event.IsLocatedEvent());
On 2016/08/03 16:12:44, sadrul wrote:
> You would DCHECK if |event| was a ui::Event, but since |event| is a
PointerEvent
> type, you don't need this DCHECK

Removed.

[sadrul, 2016-08-04 16:39:18]

A couple of nits. lgtm

https://codereview.chromium.org/2183163002/diff/140001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/140001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:630: has_pointer_watcher_ =
true;
Can you DCHECK here that |!has_pointer_watcher_| first?

https://codereview.chromium.org/2183163002/diff/140001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:636:
tree_->StopPointerWatcher();
DCHECK that |has_pointer_watcher_| is set?

[riajiang, 2016-08-04 18:21:00]

riajiang@chromium.org changed reviewers:
+ ben@chromium.org, meacer@chromium.org

[riajiang, 2016-08-04 18:21:01]

sky@, could you take a look at services/ui?

+ben@, could you take a look at content/renderer/mus and services/navigation?

+meacer@, could you take a look at the mojom?

Thanks!

https://codereview.chromium.org/2183163002/diff/140001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/140001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:630: has_pointer_watcher_ =
true;
On 2016/08/04 16:39:18, sadrul wrote:
> Can you DCHECK here that |!has_pointer_watcher_| first?

Done.

https://codereview.chromium.org/2183163002/diff/140001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:636:
tree_->StopPointerWatcher();
On 2016/08/04 16:39:18, sadrul wrote:
> DCHECK that |has_pointer_watcher_| is set?

Done.

[sky, 2016-08-04 20:40:26]

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:630:
DCHECK(!has_pointer_watcher_);
I don't think this should be a DCHECK. I think clients should be able to call
start multiple times and this does the right thing. Is there a particular reason
you want the DCHECK?

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:984: pointer_watcher_id ==
pointer_watcher_id_ && event->IsPointerEvent()) {
Shouldn't event->IsPointerEvent() be a DCHECK? When would it not be a pointer
event?

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors all events (including move events
'all events' -> 'pointer events' right? Also, explicitly say what pointer events
this includes as it isn't everything, for example, it doesn't include
enter/exit.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:85: // tree. If an event matches
the pointer watcher the observer reports it to
'matches the pointer watcher' ? There is no pointer watcher. I think you want
something like, 'For pointer events that would normally be sent to the
requesting client OnWindowInputEvent() is called, all other matching pointer
events result in OnPointerEventObserved().'

Also, you should document that there is only ever one pointer watcher active at
a given time.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:375: // also matched the active
pointer watcher for this client. The client will
Again, there is no pointer watcher (at least from the perspective of the
client). I think you want something like, '|pointer_watcher_id| is the id
supplied to StartPointerWatcher() if the event is a pointer event and the client
called StartPointerWatcher().

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:382: // Invoked when an |event|
is sent via the PointerWatcher and not targeted at
There is no PointerWatcher in this code. I think you want something like:
'Called when a pointer event that would not normally be targeted at this client
is encountered and the client called StartPointerWatcher(). See
StartPointerWatcher() for details.'

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:997: bool
WindowTree::EventMatchesPointerWatcher(const ui::Event& event) {
Position should match that of header (see style guide for specifics, basically
ordering of functions in declaration/definition should match).

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:997: bool
WindowTree::EventMatchesPointerWatcher(const ui::Event& event) {
make this function const

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1002: return pointer_watcher_want_moves_ ||
event.type() == ui::ET_POINTER_DOWN ||
If pointer_watcher_want_moves_ then shouldn't you check that the event is a
move? Otherwise you might include cancelled/exit/entered.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1177: if (pointer_watcher_id == 0) {
nit: no {}

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
File services/ui/ws/window_tree.h (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.h:493: bool has_pointer_watcher_ = false;
Why do we need both has_pointer_watcher_ and pointer_watcher_id_? If
pointer_watcher_id_ is 0 doesn't that mean there is no pointer watcher?

[riajiang, 2016-08-04 21:22:06]

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:630:
DCHECK(!has_pointer_watcher_);
On 2016/08/04 20:40:25, sky wrote:
> I don't think this should be a DCHECK. I think clients should be able to call
> start multiple times and this does the right thing. Is there a particular
reason
> you want the DCHECK?

They can call AddPointerWatcher multiple times in WindowManagerConnection but we
only call StartPointerWatcher once for the first time they call
AddPointerWatcher right?

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:984: pointer_watcher_id ==
pointer_watcher_id_ && event->IsPointerEvent()) {
On 2016/08/04 20:40:25, sky wrote:
> Shouldn't event->IsPointerEvent() be a DCHECK? When would it not be a pointer
> event?

Moved it to be a DCHECK inside this if.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors all events (including move events
On 2016/08/04 20:40:26, sky wrote:
> 'all events' -> 'pointer events' right? Also, explicitly say what pointer
events
> this includes as it isn't everything, for example, it doesn't include
> enter/exit.

Changed to "pointer events". 
But I thought we would send all of move/enter/exit if want_moves==true?

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1002: return pointer_watcher_want_moves_ ||
event.type() == ui::ET_POINTER_DOWN ||
On 2016/08/04 20:40:26, sky wrote:
> If pointer_watcher_want_moves_ then shouldn't you check that the event is a
> move? Otherwise you might include cancelled/exit/entered.

I thought we do want all pointer events except up/down in that case?

[riajiang, 2016-08-04 22:21:08]

Description was changed from

==========
mus: change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and discussions from
https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests, mus_ws_unittests
==========

to

==========
mus: Change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and
discussions from https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests, 
     mus_ws_unittests
==========

[sky, 2016-08-04 23:08:44]

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:630:
DCHECK(!has_pointer_watcher_);
On 2016/08/04 21:22:06, riajiang wrote:
> On 2016/08/04 20:40:25, sky wrote:
> > I don't think this should be a DCHECK. I think clients should be able to
call
> > start multiple times and this does the right thing. Is there a particular
> reason
> > you want the DCHECK?
> 
> They can call AddPointerWatcher multiple times in WindowManagerConnection but
we
> only call StartPointerWatcher once for the first time they call
> AddPointerWatcher right?

You shouldn't assume this code is only ever used through
WindowManagerConnection. It looks like you have TODOs in WMC to handle corner
cases there. I would like WTC to do the right thing though.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors all events (including move events
On 2016/08/04 21:22:06, riajiang wrote:
> On 2016/08/04 20:40:26, sky wrote:
> > 'all events' -> 'pointer events' right? Also, explicitly say what pointer
> events
> > this includes as it isn't everything, for example, it doesn't include
> > enter/exit.
> 
> Changed to "pointer events". 
> But I thought we would send all of move/enter/exit if want_moves==true?

Consider if you did send the exit/enter and the client doesn't know about the
window. It means the client gets an exit/enter and they don't have a clue what
the window was. We should only send enter/exit if the client knows about the
window, otherwise they should get a move.

[James Cook, 2016-08-05 16:26:46]

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors all events (including move events
On 2016/08/04 23:08:44, sky wrote:
> On 2016/08/04 21:22:06, riajiang wrote:
> > On 2016/08/04 20:40:26, sky wrote:
> > > 'all events' -> 'pointer events' right? Also, explicitly say what pointer
> > events
> > > this includes as it isn't everything, for example, it doesn't include
> > > enter/exit.
> > 
> > Changed to "pointer events". 
> > But I thought we would send all of move/enter/exit if want_moves==true?
> 
> Consider if you did send the exit/enter and the client doesn't know about the
> window. It means the client gets an exit/enter and they don't have a clue what
> the window was. We should only send enter/exit if the client knows about the
> window, otherwise they should get a move.

sky@, what do you think about having PointerWatchers never send enter/exit? (Or
was that what you were suggesting before?) Clients should already get enter/exit
on windows they know about via normal event dispatch.

Ria, this was an oversight on my part, apologies.

[sky, 2016-08-05 16:46:27]

On Fri, Aug 5, 2016 at 9:26 AM,  <jamescook@chromium.org> wrote:
>
>
https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
> File services/ui/public/interfaces/window_tree.mojom (right):
>
>
https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
> services/ui/public/interfaces/window_tree.mojom:83: // Starts the
> pointer watcher that monitors all events (including move events
> On 2016/08/04 23:08:44, sky wrote:
>> On 2016/08/04 21:22:06, riajiang wrote:
>> > On 2016/08/04 20:40:26, sky wrote:
>> > > 'all events' -> 'pointer events' right? Also, explicitly say what
> pointer
>> > events
>> > > this includes as it isn't everything, for example, it doesn't
> include
>> > > enter/exit.
>> >
>> > Changed to "pointer events".
>> > But I thought we would send all of move/enter/exit if
> want_moves==true?
>>
>> Consider if you did send the exit/enter and the client doesn't know
> about the
>> window. It means the client gets an exit/enter and they don't have a
> clue what
>> the window was. We should only send enter/exit if the client knows
> about the
>> window, otherwise they should get a move.
>
> sky@, what do you think about having PointerWatchers never send
> enter/exit? (Or was that what you were suggesting before?) Clients
> should already get enter/exit on windows they know about via normal
> event dispatch.

I'm not entirely sure that clients already get enter/exit on windows
they know about. I'm specifically thinking of the windowmanager. It
sees the whole tree but I'm not sure if it gets enter/exit on windows
it knows about but didn't create.

In hopes of not having Ria maintain this patch forever I'm ok with
dealing with this subtlety in a follow patch. The only part I would
like to see dealt with now is this comment:
https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...

  -Scott

>
> Ria, this was an oversight on my part, apologies.
>
> https://codereview.chromium.org/2183163002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[riajiang, 2016-08-05 17:36:03]

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:630:
DCHECK(!has_pointer_watcher_);
On 2016/08/04 23:08:44, sky wrote:
> On 2016/08/04 21:22:06, riajiang wrote:
> > On 2016/08/04 20:40:25, sky wrote:
> > > I don't think this should be a DCHECK. I think clients should be able to
> call
> > > start multiple times and this does the right thing. Is there a particular
> > reason
> > > you want the DCHECK?
> > 
> > They can call AddPointerWatcher multiple times in WindowManagerConnection
but
> we
> > only call StartPointerWatcher once for the first time they call
> > AddPointerWatcher right?
> 
> You shouldn't assume this code is only ever used through
> WindowManagerConnection. It looks like you have TODOs in WMC to handle corner
> cases there. I would like WTC to do the right thing though.

True makes sense. Is it correct that when clients call StartPointerWatcher
multiple times to replace the pointer watcher, we want to keep the new one?
Changed it to StopPointerWatcher for the old watcher first if there's an
existing pointer watcher.
Right now WMC only call StartPointerWatcher if it's the first time
AddPointerWatcher is called (so only one active pointer watcher at a time). The
TODO in WMC is for supporting multiple pointer watchers at the same time
(crbug.com/627146).

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors all events (including move events
On 2016/08/05 16:26:45, James Cook wrote:
> On 2016/08/04 23:08:44, sky wrote:
> > On 2016/08/04 21:22:06, riajiang wrote:
> > > On 2016/08/04 20:40:26, sky wrote:
> > > > 'all events' -> 'pointer events' right? Also, explicitly say what
pointer
> > > events
> > > > this includes as it isn't everything, for example, it doesn't include
> > > > enter/exit.
> > > 
> > > Changed to "pointer events". 
> > > But I thought we would send all of move/enter/exit if want_moves==true?
> > 
> > Consider if you did send the exit/enter and the client doesn't know about
the
> > window. It means the client gets an exit/enter and they don't have a clue
what
> > the window was. We should only send enter/exit if the client knows about the
> > window, otherwise they should get a move.
> 
> sky@, what do you think about having PointerWatchers never send enter/exit?
(Or
> was that what you were suggesting before?) Clients should already get
enter/exit
> on windows they know about via normal event dispatch.
> 
> Ria, this was an oversight on my part, apologies.

No worries. Changed it to not match enter/exit. Didn't make any changes to
PointerWatcher for now.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors all events (including move events
On 2016/08/04 23:08:44, sky wrote:
> On 2016/08/04 21:22:06, riajiang wrote:
> > On 2016/08/04 20:40:26, sky wrote:
> > > 'all events' -> 'pointer events' right? Also, explicitly say what pointer
> > events
> > > this includes as it isn't everything, for example, it doesn't include
> > > enter/exit.
> > 
> > Changed to "pointer events". 
> > But I thought we would send all of move/enter/exit if want_moves==true?
> 
> Consider if you did send the exit/enter and the client doesn't know about the
> window. It means the client gets an exit/enter and they don't have a clue what
> the window was. We should only send enter/exit if the client knows about the
> window, otherwise they should get a move.

Okay I see. Done.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:85: // tree. If an event matches
the pointer watcher the observer reports it to
On 2016/08/04 20:40:26, sky wrote:
> 'matches the pointer watcher' ? There is no pointer watcher. I think you want
> something like, 'For pointer events that would normally be sent to the
> requesting client OnWindowInputEvent() is called, all other matching pointer
> events result in OnPointerEventObserved().'
> 
> Also, you should document that there is only ever one pointer watcher active
at
> a given time.

Done and done.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:375: // also matched the active
pointer watcher for this client. The client will
On 2016/08/04 20:40:26, sky wrote:
> Again, there is no pointer watcher (at least from the perspective of the
> client). I think you want something like, '|pointer_watcher_id| is the id
> supplied to StartPointerWatcher() if the event is a pointer event and the
client
> called StartPointerWatcher().

Done.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:382: // Invoked when an |event|
is sent via the PointerWatcher and not targeted at
On 2016/08/04 20:40:26, sky wrote:
> There is no PointerWatcher in this code. I think you want something like:
> 'Called when a pointer event that would not normally be targeted at this
client
> is encountered and the client called StartPointerWatcher(). See
> StartPointerWatcher() for details.'

Done.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:997: bool
WindowTree::EventMatchesPointerWatcher(const ui::Event& event) {
On 2016/08/04 20:40:26, sky wrote:
> Position should match that of header (see style guide for specifics, basically
> ordering of functions in declaration/definition should match).

Done.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:997: bool
WindowTree::EventMatchesPointerWatcher(const ui::Event& event) {
On 2016/08/04 20:40:26, sky wrote:
> make this function const

Done.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1002: return pointer_watcher_want_moves_ ||
event.type() == ui::ET_POINTER_DOWN ||
On 2016/08/04 20:40:26, sky wrote:
> If pointer_watcher_want_moves_ then shouldn't you check that the event is a
> move? Otherwise you might include cancelled/exit/entered.

Done.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1177: if (pointer_watcher_id == 0) {
On 2016/08/04 20:40:26, sky wrote:
> nit: no {}

There's a return after StopPointerWatcher.

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
File services/ui/ws/window_tree.h (right):

https://codereview.chromium.org/2183163002/diff/160001/services/ui/ws/window_...
services/ui/ws/window_tree.h:493: bool has_pointer_watcher_ = false;
On 2016/08/04 20:40:26, sky wrote:
> Why do we need both has_pointer_watcher_ and pointer_watcher_id_? If
> pointer_watcher_id_ is 0 doesn't that mean there is no pointer watcher?

Removed.

[sky, 2016-08-05 18:06:51]

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:632: has_pointer_watcher_ =
true;
Sorry, missed this earlier. Isn't pointer_watcher_id_ enough to know if you have
a pointer watcher? If it's 0 no watcher, right?

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:633: pointer_watcher_id_++;
Make sure to skip over 0 if you wrap.

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors pointer events (monitor pointer
How about changing the () to:
(up/down events if |wants_move| if false, up/down and move if |wants_move| is
true).

https://codereview.chromium.org/2183163002/diff/180001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/180001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1010: return !pointer_watcher_want_moves_ &&
(event.type() == ui::ET_POINTER_DOWN ||
Why do you have the !pointer_watcher_wants_moves_ here? Don't you just want the
type == down/up?

https://codereview.chromium.org/2183163002/diff/180001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1180: StopPointerWatcher();
Please also log here as the client shouldn't really be calling start with 0. I'm
specifically thinking of something like:

DVLOG(1) << "StartPointerWatcher called with 0"

[riajiang, 2016-08-05 19:23:59]

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:632: has_pointer_watcher_ =
true;
On 2016/08/05 18:06:50, sky wrote:
> Sorry, missed this earlier. Isn't pointer_watcher_id_ enough to know if you
have
> a pointer watcher? If it's 0 no watcher, right?

As discussed, we'll keep has_pointer_watcher since we don't reset
pointer_watcher_id to 0 (otherwise all pointer watchers would have the same id
1) when StopPointerWatcher is called.

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/cpp...
services/ui/public/cpp/lib/window_tree_client.cc:633: pointer_watcher_id_++;
On 2016/08/05 18:06:50, sky wrote:
> Make sure to skip over 0 if you wrap.

Done.

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2183163002/diff/180001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:83: // Starts the pointer
watcher that monitors pointer events (monitor pointer
On 2016/08/05 18:06:50, sky wrote:
> How about changing the () to:
> (up/down events if |wants_move| if false, up/down and move if |wants_move| is
> true).

Done.

https://codereview.chromium.org/2183163002/diff/180001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2183163002/diff/180001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1010: return !pointer_watcher_want_moves_ &&
(event.type() == ui::ET_POINTER_DOWN ||
On 2016/08/05 18:06:51, sky wrote:
> Why do you have the !pointer_watcher_wants_moves_ here? Don't you just want
the
> type == down/up?

Oh right! Removed.

https://codereview.chromium.org/2183163002/diff/180001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1180: StopPointerWatcher();
On 2016/08/05 18:06:51, sky wrote:
> Please also log here as the client shouldn't really be calling start with 0.
I'm
> specifically thinking of something like:
> 
> DVLOG(1) << "StartPointerWatcher called with 0"

Done.

[sky, 2016-08-05 20:20:11]

Yay! LGTM

[meacer, 2016-08-06 00:21:01]

mojom lgtm

[riajiang, 2016-08-08 14:57:43]

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

[commit-bot: I haz the power, 2016-08-08 14:58:02]

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-08 16:02:21]

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

[commit-bot: I haz the power, 2016-08-08 16:02:23]

Dry run: This issue passed the CQ dry run.

[Ben Goodger (Google), 2016-08-09 15:20:41]

lgtm

[riajiang, 2016-08-09 15:36:49]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-08-09 15:36:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2183163002/#ps200001
(title: ".")

[commit-bot: I haz the power, 2016-08-09 15:37: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-08-09 15:40:18]

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

[commit-bot: I haz the power, 2016-08-09 15:40:20]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[riajiang, 2016-08-09 16:45:29]

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

[commit-bot: I haz the power, 2016-08-09 16:46: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-09 17:44:59]

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

[commit-bot: I haz the power, 2016-08-09 17:45:00]

Dry run: This issue passed the CQ dry run.

[riajiang, 2016-08-09 17:46:42]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-08-09 17:46:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sadrul@chromium.org, sky@chromium.org,
meacer@chromium.org, ben@chromium.org
Link to the patchset: https://codereview.chromium.org/2183163002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-09 17:47:31]

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-09 17:53:04]

Description was changed from

==========
mus: Change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and
discussions from https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests, 
     mus_ws_unittests
==========

to

==========
mus: Change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and
discussions from https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests, 
     mus_ws_unittests
==========

[commit-bot: I haz the power, 2016-08-09 17:53:06]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-09 17:58:46]

Description was changed from

==========
mus: Change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and
discussions from https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests, 
     mus_ws_unittests
==========

to

==========
mus: Change PointerWatcher to observe all pointer events, with moves optional.

Based on https://codereview.chromium.org/2180683003/ and
discussions from https://codereview.chromium.org/2163453002/.

BUG=628663, 631126
TEST=views_mus_unittests, mus_public_unittests, mash_unittests,
     mus_ws_unittests

Committed: https://crrev.com/13661fe1ec3d5083450e45a25401b1ec93a61d85
Cr-Commit-Position: refs/heads/master@{#410734}
==========

[commit-bot: I haz the power, 2016-08-09 17:58:48]

Patchset 12 (id:??) landed as
https://crrev.com/13661fe1ec3d5083450e45a25401b1ec93a61d85
Cr-Commit-Position: refs/heads/master@{#410734}