mus: Change PointerWatcher to watch for all events.

mus: Change PointerWatcher to watch for all events.

BUG=628663
TEST=views_mus_unittests

[riajiang, 2016-07-18 21:25:42]

riajiang@chromium.org changed reviewers:
+ sadrul@chromium.org

[riajiang, 2016-07-18 21:34:50]

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

[commit-bot: I haz the power, 2016-07-18 21:36:01]

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-07-18 22:28:01]

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

[commit-bot: I haz the power, 2016-07-18 22:28:02]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-07-19 00:18:03]

https://codereview.chromium.org/2163453002/diff/40001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection.h (right):

https://codereview.chromium.org/2163453002/diff/40001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.h:68: void
AddPointerWatcher(PointerWatcher* watcher);
Rename to AddPointerDownWatcher(PointerDownWatcher*)

https://codereview.chromium.org/2163453002/diff/40001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.h:75: void
RemoveMouseEventWatcher(PointerEventWatcher* watcher);
Hm, I don't like having two separate methods for install PointerEventWatchers.
Can we instead have a single AddPointerEventWatcher(watcher,
ui::EventPointerType) instead?

https://codereview.chromium.org/2163453002/diff/40001/ui/views/pointer_event_...
File ui/views/pointer_event_watcher.h (right):

https://codereview.chromium.org/2163453002/diff/40001/ui/views/pointer_event_...
ui/views/pointer_event_watcher.h:29: class VIEWS_EXPORT PointerEventWatcher {
This is going to be fairly confusing with PointerWatcher. Let's rename that one
to PointerDownWatcher.

https://codereview.chromium.org/2163453002/diff/40001/ui/views/pointer_event_...
ui/views/pointer_event_watcher.h:35: Widget* target) = 0;
Let's go with a single OnPointerEventObserved(). If the watcher wants to treat
mouse vs. touch separately, it can easily do that.

[sadrul, 2016-07-19 14:58:15]

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

[sadrul, 2016-07-19 14:58:16]

+sky@ for his thoughts on renaming PointerWatcher* to PointerDownWatcher*,
TouchEventWatcher to PointerEventWatcher, and having a single
WindowManagerConnection::AddPointerEventWatcher(watcher, ui::EventPointerType),
instead of separate AddMouseEventWatcher and AddTouchEventWatcher.

[sky, 2016-07-19 17:59:35]

On 2016/07/19 14:58:16, sadrul wrote:
> +sky@ for his thoughts on renaming PointerWatcher* to PointerDownWatcher*,
> TouchEventWatcher to PointerEventWatcher, and having a single
> WindowManagerConnection::AddPointerEventWatcher(watcher,
ui::EventPointerType),
> instead of separate AddMouseEventWatcher and AddTouchEventWatcher.

SGTM

[riajiang, 2016-07-19 18:46:34]

https://codereview.chromium.org/2163453002/diff/40001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection.h (right):

https://codereview.chromium.org/2163453002/diff/40001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.h:68: void
AddPointerWatcher(PointerWatcher* watcher);
On 2016/07/19 00:18:03, sadrul wrote:
> Rename to AddPointerDownWatcher(PointerDownWatcher*)

Will do the renaming in another cl.

https://codereview.chromium.org/2163453002/diff/40001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.h:75: void
RemoveMouseEventWatcher(PointerEventWatcher* watcher);
On 2016/07/19 00:18:03, sadrul wrote:
> Hm, I don't like having two separate methods for install PointerEventWatchers.
> Can we instead have a single AddPointerEventWatcher(watcher,
> ui::EventPointerType) instead?

Done.

https://codereview.chromium.org/2163453002/diff/40001/ui/views/pointer_event_...
File ui/views/pointer_event_watcher.h (right):

https://codereview.chromium.org/2163453002/diff/40001/ui/views/pointer_event_...
ui/views/pointer_event_watcher.h:29: class VIEWS_EXPORT PointerEventWatcher {
On 2016/07/19 00:18:03, sadrul wrote:
> This is going to be fairly confusing with PointerWatcher. Let's rename that
one
> to PointerDownWatcher.

Will do the renaming in another cl.

https://codereview.chromium.org/2163453002/diff/40001/ui/views/pointer_event_...
ui/views/pointer_event_watcher.h:35: Widget* target) = 0;
On 2016/07/19 00:18:03, sadrul wrote:
> Let's go with a single OnPointerEventObserved(). If the watcher wants to treat
> mouse vs. touch separately, it can easily do that.

Done.

[sadrul, 2016-07-20 16:14:54]

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

https://codereview.chromium.org/2163453002/diff/60001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.h:105:
base::ObserverList<PointerEventWatcher, true> mouse_event_watchers_;
Can this be an ObserverList<> pointer_event_watchers_, and a
ui::EventPointerType, instead of two observer lists?

[James Cook, 2016-07-20 17:38:54]

jamescook@chromium.org changed reviewers:
+ jamescook@chromium.org

[James Cook, 2016-07-20 17:38:54]

drive-by since I just saw the rename CL land... It seems confusing to me to have
two watchers, one for all pointer events and one for just pointer down.

The reason my original PointerWatcher only watched for down was because that was
all I needed, and we didn't want the overhead of sending hover events.

What do you guys think about having a single "pointerwatcher" that sends
everything, with an option for whether hover events are included? The overhead
of sending the various "up" events to ash seems low, and other clients like
touch hud could request hover events.

[sky, 2016-07-20 19:38:50]

+1 to the option

On Wed, Jul 20, 2016 at 10:38 AM,  <jamescook@chromium.org> wrote:
> drive-by since I just saw the rename CL land... It seems confusing to me to
> have
> two watchers, one for all pointer events and one for just pointer down.
>
> The reason my original PointerWatcher only watched for down was because that
> was
> all I needed, and we didn't want the overhead of sending hover events.
>
> What do you guys think about having a single "pointerwatcher" that sends
> everything, with an option for whether hover events are included? The
> overhead
> of sending the various "up" events to ash seems low, and other clients like
> touch hud could request hover events.
>
>
> https://codereview.chromium.org/2163453002/

-- 
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.

[sadrul, 2016-07-20 19:43:33]

On 2016/07/20 19:38:50, sky wrote:
> +1 to the option
> 
> On Wed, Jul 20, 2016 at 10:38 AM,  <mailto:jamescook@chromium.org> wrote:
> > drive-by since I just saw the rename CL land... It seems confusing to me to
> > have
> > two watchers, one for all pointer events and one for just pointer down.
> >
> > The reason my original PointerWatcher only watched for down was because that
> > was
> > all I needed, and we didn't want the overhead of sending hover events.
> >
> > What do you guys think about having a single "pointerwatcher" that sends
> > everything, with an option for whether hover events are included? The
> > overhead
> > of sending the various "up" events to ash seems low, and other clients like
> > touch hud could request hover events.

Do you think we should change EventMatcher to accommodate this? Or add some
separate flag to SetEventObserver()?

[sky, 2016-07-20 19:54:34]

What James outlines doesn't require any changes to SetEventObserver.
Maybe I'm missing something?

On Wed, Jul 20, 2016 at 12:43 PM,  <sadrul@chromium.org> wrote:
> On 2016/07/20 19:38:50, sky wrote:
>> +1 to the option
>>
>> On Wed, Jul 20, 2016 at 10:38 AM, <mailto:jamescook@chromium.org> wrote:
>> > drive-by since I just saw the rename CL land... It seems confusing to me
>> > to
>> > have
>> > two watchers, one for all pointer events and one for just pointer down.
>> >
>> > The reason my original PointerWatcher only watched for down was because
>> > that
>> > was
>> > all I needed, and we didn't want the overhead of sending hover events.
>> >
>> > What do you guys think about having a single "pointerwatcher" that sends
>> > everything, with an option for whether hover events are included? The
>> > overhead
>> > of sending the various "up" events to ash seems low, and other clients
>> > like
>> > touch hud could request hover events.
>
> Do you think we should change EventMatcher to accommodate this? Or add some
> separate flag to SetEventObserver()?
>
> https://codereview.chromium.org/2163453002/

-- 
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.

[sadrul, 2016-07-20 20:18:06]

On 2016/07/20 19:54:34, sky wrote:
> What James outlines doesn't require any changes to SetEventObserver.
> Maybe I'm missing something?

I had two interpretations of the proposal, one of which isn't making any sense
anymore even to me, so I was missing something :)

My understanding of the proposal: have a single
WindowManagerConnection::AddPointerWatcher(watcher, source = MOUSE|TOUCH|ALL,
want_moves = true|false), which then creates an EventMatcher (with type_matcher
set to POINTER_DOWN only if want_moves is set to false), and sends that over
with SetEventObserver(). Do I have this right?

[James Cook, 2016-07-20 20:26:52]

On 2016/07/20 20:18:06, sadrul wrote:
> On 2016/07/20 19:54:34, sky wrote:
> > What James outlines doesn't require any changes to SetEventObserver.
> > Maybe I'm missing something?
> 
> I had two interpretations of the proposal, one of which isn't making any sense
> anymore even to me, so I was missing something :)
> 
> My understanding of the proposal: have a single
> WindowManagerConnection::AddPointerWatcher(watcher, source = MOUSE|TOUCH|ALL,
> want_moves = true|false), which then creates an EventMatcher (with
type_matcher
> set to POINTER_DOWN only if want_moves is set to false), and sends that over
> with SetEventObserver(). Do I have this right?

That could work.

Frankly, I wonder if we need an EventMatcher at all. PointerWatcher /
EventObservers are already a pretty special case in that they explicitly ignore
keyboard event types, but want almost all pointer types. I would be OK with
replacing SetEventObserver() with something like StartPointerWatcher(bool
want_moves). The client would then get all mouse/touch events and could do its
own filtering.

I just have a feeling we're going to quickly get to a place where
EventHandler/EventFilter code in ash is going to need touch up/down, mouse
up/down, etc. so the extra filtering of EventMatcher may not be that helpful.

But I'm OK with keeping EventMatcher too.

[sky, 2016-07-20 22:21:48]

Ash will need to see and consume certain sets of events in some
situations. For example, WindowCycleEventFilterAura wants to see *all*
key events, consuming until a certain sequence is encountered. The
maximize code wants to see and possibly consume touch events. So,
there are cases that are more conveniently expressed with an
EventHandler.

  -Scott

On Wed, Jul 20, 2016 at 1:26 PM,  <jamescook@chromium.org> wrote:
> On 2016/07/20 20:18:06, sadrul wrote:
>> On 2016/07/20 19:54:34, sky wrote:
>> > What James outlines doesn't require any changes to SetEventObserver.
>> > Maybe I'm missing something?
>>
>> I had two interpretations of the proposal, one of which isn't making any
>> sense
>> anymore even to me, so I was missing something :)
>>
>> My understanding of the proposal: have a single
>> WindowManagerConnection::AddPointerWatcher(watcher, source =
>> MOUSE|TOUCH|ALL,
>> want_moves = true|false), which then creates an EventMatcher (with
> type_matcher
>> set to POINTER_DOWN only if want_moves is set to false), and sends that
>> over
>> with SetEventObserver(). Do I have this right?
>
> That could work.
>
> Frankly, I wonder if we need an EventMatcher at all. PointerWatcher /
> EventObservers are already a pretty special case in that they explicitly
> ignore
> keyboard event types, but want almost all pointer types. I would be OK with
> replacing SetEventObserver() with something like StartPointerWatcher(bool
> want_moves). The client would then get all mouse/touch events and could do
> its
> own filtering.
>
> I just have a feeling we're going to quickly get to a place where
> EventHandler/EventFilter code in ash is going to need touch up/down, mouse
> up/down, etc. so the extra filtering of EventMatcher may not be that
> helpful.
>
> But I'm OK with keeping EventMatcher too.
>
>
> https://codereview.chromium.org/2163453002/

-- 
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-07-21 18:26:16]

On 2016/07/20 22:21:48, sky wrote:
> Ash will need to see and consume certain sets of events in some
> situations. For example, WindowCycleEventFilterAura wants to see *all*
> key events, consuming until a certain sequence is encountered. The
> maximize code wants to see and possibly consume touch events. So,
> there are cases that are more conveniently expressed with an
> EventHandler.
> 
>   -Scott
> 
> On Wed, Jul 20, 2016 at 1:26 PM,  <mailto:jamescook@chromium.org> wrote:
> > On 2016/07/20 20:18:06, sadrul wrote:
> >> On 2016/07/20 19:54:34, sky wrote:
> >> > What James outlines doesn't require any changes to SetEventObserver.
> >> > Maybe I'm missing something?
> >>
> >> I had two interpretations of the proposal, one of which isn't making any
> >> sense
> >> anymore even to me, so I was missing something :)
> >>
> >> My understanding of the proposal: have a single
> >> WindowManagerConnection::AddPointerWatcher(watcher, source =
> >> MOUSE|TOUCH|ALL,
> >> want_moves = true|false), which then creates an EventMatcher (with
> > type_matcher
> >> set to POINTER_DOWN only if want_moves is set to false), and sends that
> >> over
> >> with SetEventObserver(). Do I have this right?
> >
> > That could work.
> >
> > Frankly, I wonder if we need an EventMatcher at all. PointerWatcher /
> > EventObservers are already a pretty special case in that they explicitly
> > ignore
> > keyboard event types, but want almost all pointer types. I would be OK with
> > replacing SetEventObserver() with something like StartPointerWatcher(bool
> > want_moves). The client would then get all mouse/touch events and could do
> > its
> > own filtering.
> >
> > I just have a feeling we're going to quickly get to a place where
> > EventHandler/EventFilter code in ash is going to need touch up/down, mouse
> > up/down, etc. so the extra filtering of EventMatcher may not be that
> > helpful.
> >
> > But I'm OK with keeping EventMatcher too.
> >
> >
> > https://codereview.chromium.org/2163453002/
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 

I'm going to make another cl that reverses the renaming. Then in this cl delete
PointerEventWatcher, change PointerWatcher to watch for all events and use
AddPointerWatcher(watcher, source = MOUSE|TOUCH|ALL, want_moves = true|false) in
WMC instead. Am I understanding this correct?

[James Cook, 2016-07-21 19:06:30]

On 2016/07/21 18:26:16, riajiang wrote:
> On 2016/07/20 22:21:48, sky wrote:
> > Ash will need to see and consume certain sets of events in some
> > situations. For example, WindowCycleEventFilterAura wants to see *all*
> > key events, consuming until a certain sequence is encountered. The
> > maximize code wants to see and possibly consume touch events. So,
> > there are cases that are more conveniently expressed with an
> > EventHandler.
> > 
> >   -Scott
> > 
> > On Wed, Jul 20, 2016 at 1:26 PM,  <mailto:jamescook@chromium.org> wrote:
> > > On 2016/07/20 20:18:06, sadrul wrote:
> > >> On 2016/07/20 19:54:34, sky wrote:
> > >> > What James outlines doesn't require any changes to SetEventObserver.
> > >> > Maybe I'm missing something?
> > >>
> > >> I had two interpretations of the proposal, one of which isn't making any
> > >> sense
> > >> anymore even to me, so I was missing something :)
> > >>
> > >> My understanding of the proposal: have a single
> > >> WindowManagerConnection::AddPointerWatcher(watcher, source =
> > >> MOUSE|TOUCH|ALL,
> > >> want_moves = true|false), which then creates an EventMatcher (with
> > > type_matcher
> > >> set to POINTER_DOWN only if want_moves is set to false), and sends that
> > >> over
> > >> with SetEventObserver(). Do I have this right?
> > >
> > > That could work.
> > >
> > > Frankly, I wonder if we need an EventMatcher at all. PointerWatcher /
> > > EventObservers are already a pretty special case in that they explicitly
> > > ignore
> > > keyboard event types, but want almost all pointer types. I would be OK
with
> > > replacing SetEventObserver() with something like StartPointerWatcher(bool
> > > want_moves). The client would then get all mouse/touch events and could do
> > > its
> > > own filtering.
> > >
> > > I just have a feeling we're going to quickly get to a place where
> > > EventHandler/EventFilter code in ash is going to need touch up/down, mouse
> > > up/down, etc. so the extra filtering of EventMatcher may not be that
> > > helpful.
> > >
> > > But I'm OK with keeping EventMatcher too.
> > >
> > >
> > > https://codereview.chromium.org/2163453002/
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> 
> I'm going to make another cl that reverses the renaming. Then in this cl
delete
> PointerEventWatcher, change PointerWatcher to watch for all events and use
> AddPointerWatcher(watcher, source = MOUSE|TOUCH|ALL, want_moves = true|false)
in
> WMC instead. Am I understanding this correct?

SGTM. Sadrul and/or Sky?

[sky, 2016-07-21 19:54:37]

SGTM too

On Thu, Jul 21, 2016 at 12:06 PM,  <jamescook@chromium.org> wrote:
> On 2016/07/21 18:26:16, riajiang wrote:
>> On 2016/07/20 22:21:48, sky wrote:
>> > Ash will need to see and consume certain sets of events in some
>> > situations. For example, WindowCycleEventFilterAura wants to see *all*
>> > key events, consuming until a certain sequence is encountered. The
>> > maximize code wants to see and possibly consume touch events. So,
>> > there are cases that are more conveniently expressed with an
>> > EventHandler.
>> >
>> > -Scott
>> >
>> > On Wed, Jul 20, 2016 at 1:26 PM, <mailto:jamescook@chromium.org> wrote:
>> > > On 2016/07/20 20:18:06, sadrul wrote:
>> > >> On 2016/07/20 19:54:34, sky wrote:
>> > >> > What James outlines doesn't require any changes to
>> > >> > SetEventObserver.
>> > >> > Maybe I'm missing something?
>> > >>
>> > >> I had two interpretations of the proposal, one of which isn't making
>> > >> any
>> > >> sense
>> > >> anymore even to me, so I was missing something :)
>> > >>
>> > >> My understanding of the proposal: have a single
>> > >> WindowManagerConnection::AddPointerWatcher(watcher, source =
>> > >> MOUSE|TOUCH|ALL,
>> > >> want_moves = true|false), which then creates an EventMatcher (with
>> > > type_matcher
>> > >> set to POINTER_DOWN only if want_moves is set to false), and sends
>> > >> that
>> > >> over
>> > >> with SetEventObserver(). Do I have this right?
>> > >
>> > > That could work.
>> > >
>> > > Frankly, I wonder if we need an EventMatcher at all. PointerWatcher /
>> > > EventObservers are already a pretty special case in that they
>> > > explicitly
>> > > ignore
>> > > keyboard event types, but want almost all pointer types. I would be OK
> with
>> > > replacing SetEventObserver() with something like
>> > > StartPointerWatcher(bool
>> > > want_moves). The client would then get all mouse/touch events and
>> > > could do
>> > > its
>> > > own filtering.
>> > >
>> > > I just have a feeling we're going to quickly get to a place where
>> > > EventHandler/EventFilter code in ash is going to need touch up/down,
>> > > mouse
>> > > up/down, etc. so the extra filtering of EventMatcher may not be that
>> > > helpful.
>> > >
>> > > But I'm OK with keeping EventMatcher too.
>> > >
>> > >
>> > > https://codereview.chromium.org/2163453002/
>> >
>> > --
>> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>> >
>>
>> I'm going to make another cl that reverses the renaming. Then in this cl
> delete
>> PointerEventWatcher, change PointerWatcher to watch for all events and use
>> AddPointerWatcher(watcher, source = MOUSE|TOUCH|ALL, want_moves =
>> true|false)
> in
>> WMC instead. Am I understanding this correct?
>
> SGTM. Sadrul and/or Sky?
>
>
> https://codereview.chromium.org/2163453002/

-- 
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-07-22 20:00:09]

Description was changed from

==========
mus: Add watcher for all mouse events.

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

to

==========
mus: Change PointerWatcher to watch for all events.

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

[riajiang, 2016-07-22 20:38:44]

PTAL. Thanks!

[James Cook, 2016-07-22 22:06:01]

On 2016/07/22 20:38:44, riajiang wrote:
> PTAL. Thanks!

This is a step in the right direction, but it isn't what I thought you were
going to build. I thought you would introduce AddPointerWatcher(watcher, source
= MOUSE|TOUCH|ALL, want_moves = true|false), which I could call multiple times
with any combination of source/want_moves, and my PointerWatchers would be
notified of the *all* the mouse/touch events they requested, plus or minus
moves.

What you have will work in the narrow cases used by touch-hud and ash
close-bubble-on-click, but I think it's confusing and doesn't provide us much
future flexibility.

Sadrul/Scott, WDYT?

[James Cook, 2016-07-22 22:06:26]

https://codereview.chromium.org/2163453002/diff/100001/ash/common/shelf/overf...
File ash/common/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/2163453002/diff/100001/ash/common/shelf/overf...
ash/common/shelf/overflow_bubble.cc:78: ProcessPressedEvent(location_in_screen);
For example, I would expect you to have to check the event type here. The
AddPointerWatcher call above doesn't specify any filtering specific to
mousedown.

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

https://codereview.chromium.org/2163453002/diff/100001/ui/views/mus/window_ma...
ui/views/mus/window_manager_connection.cc:122: matcher->type_matcher->type =
ui::mojom::EventType::POINTER_DOWN;
Likewise, it's very odd that want_moves false implies down events only.

[sadrul, 2016-07-22 22:16:42]

On 2016/07/22 22:06:01, James Cook wrote:
> On 2016/07/22 20:38:44, riajiang wrote:
> > PTAL. Thanks!
> 
> This is a step in the right direction, but it isn't what I thought you were
> going to build. I thought you would introduce AddPointerWatcher(watcher,
source
> = MOUSE|TOUCH|ALL, want_moves = true|false), which I could call multiple times
> with any combination of source/want_moves, and my PointerWatchers would be
> notified of the *all* the mouse/touch events they requested, plus or minus
> moves.
> 
> What you have will work in the narrow cases used by touch-hud and ash
> close-bubble-on-click, but I think it's confusing and doesn't provide us much
> future flexibility.

Sounds like you are suggesting supporting event-observers for multiple types of
EventMatcher (i.e. crbug.com/627146). It's not clear what kind of flexibility we
would actually need. That's why we are punting on supporting multiple types of
watchers from the same client for now (crbug.com/627146)

[James Cook, 2016-07-22 22:37:26]

On 2016/07/22 22:16:42, sadrul wrote:
> On 2016/07/22 22:06:01, James Cook wrote:
> > On 2016/07/22 20:38:44, riajiang wrote:
> > > PTAL. Thanks!
> > 
> > This is a step in the right direction, but it isn't what I thought you were
> > going to build. I thought you would introduce AddPointerWatcher(watcher,
> source
> > = MOUSE|TOUCH|ALL, want_moves = true|false), which I could call multiple
times
> > with any combination of source/want_moves, and my PointerWatchers would be
> > notified of the *all* the mouse/touch events they requested, plus or minus
> > moves.
> > 
> > What you have will work in the narrow cases used by touch-hud and ash
> > close-bubble-on-click, but I think it's confusing and doesn't provide us
much
> > future flexibility.
> 
> Sounds like you are suggesting supporting event-observers for multiple types
of
> EventMatcher (i.e. crbug.com/627146). It's not clear what kind of flexibility
we
> would actually need. That's why we are punting on supporting multiple types of
> watchers from the same client for now (crbug.com/627146)

This is why I think we should just send all non-hover pointer events to the
client.

Right now the code is very confusing, in that |wants_moves| false implies
something about the type of events you are requesting. (I missed that detail in
your original comment.) It also means we have a function
PointerWatcher::OnPointerEventObserved() that sometimes gets called with all
events, sometime with press only, and the high-level AddPointerWatcher() call
doesn't provide any hints that you're just going to get press events.

[sadrul, 2016-07-23 00:22:54]

On 2016/07/22 22:37:26, James Cook wrote:
> On 2016/07/22 22:16:42, sadrul wrote:
> > On 2016/07/22 22:06:01, James Cook wrote:
> > > On 2016/07/22 20:38:44, riajiang wrote:
> > > > PTAL. Thanks!
> > > 
> > > This is a step in the right direction, but it isn't what I thought you
were
> > > going to build. I thought you would introduce AddPointerWatcher(watcher,
> > source
> > > = MOUSE|TOUCH|ALL, want_moves = true|false), which I could call multiple
> times
> > > with any combination of source/want_moves, and my PointerWatchers would be
> > > notified of the *all* the mouse/touch events they requested, plus or minus
> > > moves.
> > > 
> > > What you have will work in the narrow cases used by touch-hud and ash
> > > close-bubble-on-click, but I think it's confusing and doesn't provide us
> much
> > > future flexibility.
> > 
> > Sounds like you are suggesting supporting event-observers for multiple types
> of
> > EventMatcher (i.e. crbug.com/627146). It's not clear what kind of
flexibility
> we
> > would actually need. That's why we are punting on supporting multiple types
of
> > watchers from the same client for now (crbug.com/627146)
> 
> This is why I think we should just send all non-hover pointer events to the
> client.
> 
> Right now the code is very confusing, in that |wants_moves| false implies
> something about the type of events you are requesting. (I missed that detail
in
> your original comment.) It also means we have a function
> PointerWatcher::OnPointerEventObserved() that sometimes gets called with all
> events, sometime with press only, and the high-level AddPointerWatcher() call
> doesn't provide any hints that you're just going to get press events.

Yes, this is why we originally went with separate PointerDownWatcher and
PointerEventWatcher interfaces.

[James Cook, 2016-07-23 06:57:08]

On 2016/07/23 00:22:54, sadrul wrote:
> On 2016/07/22 22:37:26, James Cook wrote:
> > On 2016/07/22 22:16:42, sadrul wrote:
> > > On 2016/07/22 22:06:01, James Cook wrote:
> > > > On 2016/07/22 20:38:44, riajiang wrote:
> > > > > PTAL. Thanks!
> > > > 
> > > > This is a step in the right direction, but it isn't what I thought you
> were
> > > > going to build. I thought you would introduce AddPointerWatcher(watcher,
> > > source
> > > > = MOUSE|TOUCH|ALL, want_moves = true|false), which I could call multiple
> > times
> > > > with any combination of source/want_moves, and my PointerWatchers would
be
> > > > notified of the *all* the mouse/touch events they requested, plus or
minus
> > > > moves.
> > > > 
> > > > What you have will work in the narrow cases used by touch-hud and ash
> > > > close-bubble-on-click, but I think it's confusing and doesn't provide us
> > much
> > > > future flexibility.
> > > 
> > > Sounds like you are suggesting supporting event-observers for multiple
types
> > of
> > > EventMatcher (i.e. crbug.com/627146). It's not clear what kind of
> flexibility
> > we
> > > would actually need. That's why we are punting on supporting multiple
types
> of
> > > watchers from the same client for now (crbug.com/627146)
> > 
> > This is why I think we should just send all non-hover pointer events to the
> > client.
> > 
> > Right now the code is very confusing, in that |wants_moves| false implies
> > something about the type of events you are requesting. (I missed that detail
> in
> > your original comment.) It also means we have a function
> > PointerWatcher::OnPointerEventObserved() that sometimes gets called with all
> > events, sometime with press only, and the high-level AddPointerWatcher()
call
> > doesn't provide any hints that you're just going to get press events.
> 
> Yes, this is why we originally went with separate PointerDownWatcher and
> PointerEventWatcher interfaces.

What I had in mind was something like this:
https://codereview.chromium.org/2180683003

Let's talk in the meeting Monday about how to proceed.