Eager Gesture Recognition on Aura

Eager Gesture Recognition on Aura

This enables eager gesture recognition on Aura for the unified gesture
detector.

BUG=332418
TEST=GestureRecognizer/GestureRecognizerTest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288236

[tdresser, 2014-07-16 18:04:23]

sadrul, can you take a look?

[sadrul, 2014-07-16 21:07:49]

https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1923:
window_->GetHost()->dispatcher()->OnForwardingTouchEvent(event, window_);
Can you explain what OnForwardingTouchEvent does, and why this is necessary?
(e.g. the code in WindowEventDispatcher::PostDispatchEvent() should be
sufficient, except it would ignore event.result())

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:517: }
Should this be something like:
  if (unified && !consumed) {
    gestures_.reset(AckTouchEventForGesture(....));
  else
    gestures_.reset(ProcessTouchEventForGesture(....));

to avoid creating gestures in line 508, and then discarding them in line 514?

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.h:96: // Called before the event is forwarded to
the |host_|.
Is this comment correct?

[tdresser, 2014-07-17 17:05:28]

On 2014/07/16 21:07:49, sadrul wrote:
>
https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:1923:
> window_->GetHost()->dispatcher()->OnForwardingTouchEvent(event, window_);
> Can you explain what OnForwardingTouchEvent does, and why this is necessary?
> (e.g. the code in WindowEventDispatcher::PostDispatchEvent() should be
> sufficient, except it would ignore event.result())
> 
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> File ui/aura/window_event_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> ui/aura/window_event_dispatcher.cc:517: }
> Should this be something like:
>   if (unified && !consumed) {
>     gestures_.reset(AckTouchEventForGesture(....));
>   else
>     gestures_.reset(ProcessTouchEventForGesture(....));
> 
> to avoid creating gestures in line 508, and then discarding them in line 514?
> 
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> File ui/aura/window_event_dispatcher.h (right):
> 
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> ui/aura/window_event_dispatcher.h:96: // Called before the event is forwarded
to
> the |host_|.
> Is this comment correct?

One difference between using OnForwardingTouchEvent and PostDispatchEvent occurs
when a touch move event with no delta is sent.
PostDispatchEvent will get an event with event.result() == ui::ET_CONSUMED, but
ProcessedTouchEvent will never get called, so we'd end up with our ACKs getting
out of sync.

OnForwardingTouchEvent ensures that we'll only give the GR a touch event iff
ProcessedTouchEvent will be called.

Does that sound reasonable to you? If so, I'll add some additional comments to
clarify this.

[sadrul, 2014-07-18 16:14:01]

On 2014/07/17 17:05:28, tdresser wrote:
> On 2014/07/16 21:07:49, sadrul wrote:
> >
>
https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:1923:
> > window_->GetHost()->dispatcher()->OnForwardingTouchEvent(event, window_);
> > Can you explain what OnForwardingTouchEvent does, and why this is necessary?
> > (e.g. the code in WindowEventDispatcher::PostDispatchEvent() should be
> > sufficient, except it would ignore event.result())
> > 
> >
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> > File ui/aura/window_event_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> > ui/aura/window_event_dispatcher.cc:517: }
> > Should this be something like:
> >   if (unified && !consumed) {
> >     gestures_.reset(AckTouchEventForGesture(....));
> >   else
> >     gestures_.reset(ProcessTouchEventForGesture(....));
> > 
> > to avoid creating gestures in line 508, and then discarding them in line
514?
> > 
> >
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> > File ui/aura/window_event_dispatcher.h (right):
> > 
> >
>
https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
> > ui/aura/window_event_dispatcher.h:96: // Called before the event is
forwarded
> to
> > the |host_|.
> > Is this comment correct?
> 
> One difference between using OnForwardingTouchEvent and PostDispatchEvent
occurs
> when a touch move event with no delta is sent.

Perhaps we should remove this optimization, and always forward the touch-event
to the renderer regardless of whether a touch-point has been updated or not? I
suspect GR's written in JS would benefit from this too? Or, if we decide not to
forward these events, perhaps that decision should live in the InputRouter.

> PostDispatchEvent will get an event with event.result() == ui::ET_CONSUMED,
but
> ProcessedTouchEvent will never get called, so we'd end up with our ACKs
getting
> out of sync.
> 
> OnForwardingTouchEvent ensures that we'll only give the GR a touch event iff
> ProcessedTouchEvent will be called.
> 
> Does that sound reasonable to you? If so, I'll add some additional comments to
> clarify this.

[tdresser, 2014-07-18 17:08:48]

Jared, what do you think about this?

I was initially inclined to not send 0-delta moves all the way to Javascript, as
some screens don't send 0-delta moves, and we want to isolate web developers
from differences between screens.

But I guess with buffered touch input, we could get around this by sending a
touchmove for every frame regardless of whether or not the touchscreen sent a
touchmove. I think that would be the most rational set of events from the web
developers perspective.

Are we concerned about the performance impact here? It'll be no worse than a
frame in which we do have a touchmove event, so I don't think we should be too
concerned about it.

[jdduke (slow), 2014-07-18 17:19:37]

On 2014/07/18 17:08:48, tdresser wrote:
> Jared, what do you think about this?
> 
> I was initially inclined to not send 0-delta moves all the way to Javascript,
as
> some screens don't send 0-delta moves, and we want to isolate web developers
> from differences between screens.
> 
> But I guess with buffered touch input, we could get around this by sending a
> touchmove for every frame regardless of whether or not the touchscreen sent a
> touchmove. I think that would be the most rational set of events from the web
> developers perspective.
> 
> Are we concerned about the performance impact here? It'll be no worse than a
> frame in which we do have a touchmove event, so I don't think we should be too
> concerned about it.

So if a user's finger is stationary, we're getting 100hz worth of 0-delta moves?
Honestly, I think a 0-delta touchmove is in general more surprising than *not*
getting a touchmove at all.  I wouldn't expect a mousemove either if the mouse
isn't actually moving. 

If I had a preference it would be that we suppress this as early as possible in
the pipeline, but maybe that's not possible? What are the advantages to sending
a 0-delta touchmove?

[jdduke (slow), 2014-07-18 17:24:28]

On 2014/07/18 17:19:37, jdduke wrote:
> So if a user's finger is stationary, we're getting 100hz worth of 0-delta
moves?

Where 100 is 60-ish with buffered input.

[tdresser, 2014-07-18 17:28:00]

On 2014/07/18 17:19:37, jdduke wrote:
> On 2014/07/18 17:08:48, tdresser wrote:
> > Jared, what do you think about this?
> > 
> > I was initially inclined to not send 0-delta moves all the way to
Javascript,
> as
> > some screens don't send 0-delta moves, and we want to isolate web developers
> > from differences between screens.
> > 
> > But I guess with buffered touch input, we could get around this by sending a
> > touchmove for every frame regardless of whether or not the touchscreen sent
a
> > touchmove. I think that would be the most rational set of events from the
web
> > developers perspective.
> > 
> > Are we concerned about the performance impact here? It'll be no worse than a
> > frame in which we do have a touchmove event, so I don't think we should be
too
> > concerned about it.
> 
> So if a user's finger is stationary, we're getting 100hz worth of 0-delta
moves?
> Honestly, I think a 0-delta touchmove is in general more surprising than *not*
> getting a touchmove at all.  I wouldn't expect a mousemove either if the mouse
> isn't actually moving. 
> 
> If I had a preference it would be that we suppress this as early as possible
in
> the pipeline, but maybe that's not possible? What are the advantages to
sending
> a 0-delta touchmove?

It could be useful in gesture detection in some cases (velocity estimation?).

We could filter it out arbitrarily early, though it would be somewhat more
complicated to push it earlier, as in this location we've already got the touch
history.

The difference between mouse and touch is that with mouse, I can leave it
sitting on the table, and it makes sense that I get no mouse moves.
With touch, while a finger is down, you don't really expect it to remain
completely still, but it will occasionally, depending on the sensitivity of the
touchscreen etc.

That being said, I'm not against filtering out 0-delta mouse moves.

[tdresser, 2014-07-23 14:49:52]

On 2014/07/18 17:28:00, tdresser wrote:
> On 2014/07/18 17:19:37, jdduke wrote:
> > On 2014/07/18 17:08:48, tdresser wrote:
> > > Jared, what do you think about this?
> > > 
> > > I was initially inclined to not send 0-delta moves all the way to
> Javascript,
> > as
> > > some screens don't send 0-delta moves, and we want to isolate web
developers
> > > from differences between screens.
> > > 
> > > But I guess with buffered touch input, we could get around this by sending
a
> > > touchmove for every frame regardless of whether or not the touchscreen
sent
> a
> > > touchmove. I think that would be the most rational set of events from the
> web
> > > developers perspective.
> > > 
> > > Are we concerned about the performance impact here? It'll be no worse than
a
> > > frame in which we do have a touchmove event, so I don't think we should be
> too
> > > concerned about it.
> > 
> > So if a user's finger is stationary, we're getting 100hz worth of 0-delta
> moves?
> > Honestly, I think a 0-delta touchmove is in general more surprising than
*not*
> > getting a touchmove at all.  I wouldn't expect a mousemove either if the
mouse
> > isn't actually moving. 
> > 
> > If I had a preference it would be that we suppress this as early as possible
> in
> > the pipeline, but maybe that's not possible? What are the advantages to
> sending
> > a 0-delta touchmove?
> 
> It could be useful in gesture detection in some cases (velocity estimation?).
> 
> We could filter it out arbitrarily early, though it would be somewhat more
> complicated to push it earlier, as in this location we've already got the
touch
> history.
> 
> The difference between mouse and touch is that with mouse, I can leave it
> sitting on the table, and it makes sense that I get no mouse moves.
> With touch, while a finger is down, you don't really expect it to remain
> completely still, but it will occasionally, depending on the sensitivity of
the
> touchscreen etc.
> 
> That being said, I'm not against filtering out 0-delta mouse moves.

I'm not convinced that we want to let the gesture detector prevent touch events
from being dispatched.
Does this ever happen on Android?

If this is an edge case which only occurs when we're given an abnormal event
stream, would it be reasonable to just push empty gesture packets for invalid
events, and dispatch the invalid events as usual?

The specific issue I'm running into is related to the difference between
ET_TOUCH_CANCELLED and ACTION_CANCEL.
The gesture detector, upon seeing an ACTION_CANCEL, assumes all touches have
been released, and then considers future ACTION_CANCEL events invalid.

In the GestureProviderAura, I'm currently converting all ET_TOUCH_CANCELLED
events into ACTION_CANCEL MotionEvents. This works fine without the eager GR,
because we never cancel some touch events without cancelling all of the touch
events. 

Without modifying the gesture detector, the eager GR will now prevent us from
dispatching multiple touch cancel events, which will potentially break some
things in Aura.

It would be possible to fix this specific issue, but even in general, it feels
somewhat strange to allow the gesture detector to prevent touch events from
being dispatched.

[jdduke (slow), 2014-07-23 15:10:49]

On 2014/07/23 14:49:52, tdresser wrote:
> I'm not convinced that we want to let the gesture detector prevent touch
events
> from being dispatched.
> Does this ever happen on Android?
> 
> If this is an edge case which only occurs when we're given an abnormal event
> stream, would it be reasonable to just push empty gesture packets for invalid
> events, and dispatch the invalid events as usual?
> 
> The specific issue I'm running into is related to the difference between
> ET_TOUCH_CANCELLED and ACTION_CANCEL.
> The gesture detector, upon seeing an ACTION_CANCEL, assumes all touches have
> been released, and then considers future ACTION_CANCEL events invalid.
> 
> In the GestureProviderAura, I'm currently converting all ET_TOUCH_CANCELLED
> events into ACTION_CANCEL MotionEvents. This works fine without the eager GR,
> because we never cancel some touch events without cancelling all of the touch
> events. 
> 

Who synthesizes, and when, these ET_TOUCH_CANCELLED events? Will it ever happen
that one pointer is cancelled but not another? If so, then yes we have a problem
and might need to modify the gesture detector to accommodate this.  Otherwise,
perhaps we could adjust the semantics of ET_TOUCH_CANCELLED so that it's a
broadcast for all active pointers?

> Without modifying the gesture detector, the eager GR will now prevent us from
> dispatching multiple touch cancel events, which will potentially break some
> things in Aura.
> 
> It would be possible to fix this specific issue, but even in general, it feels
> somewhat strange to allow the gesture detector to prevent touch events from
> being dispatched.

The GR doesn't really care what events get forwarded, it's the eager GR.  The
original motivation for conditionally forwarding events was preventing a partial
touch sequence from being forwarded, giving strong guarantees on "complete"
event stream delivery.  If those events aren't forwarded, the eager GR won't
expect an ack, so it drops them entirely.  I suppose we could change this
behavior, but it would be a shame to do it solely for the case of ACTION_CANCEL?

[tdresser, 2014-07-23 15:54:41]

On 2014/07/23 15:10:49, jdduke wrote:
> On 2014/07/23 14:49:52, tdresser wrote:
> > I'm not convinced that we want to let the gesture detector prevent touch
> events
> > from being dispatched.
> > Does this ever happen on Android?
> > 
> > If this is an edge case which only occurs when we're given an abnormal event
> > stream, would it be reasonable to just push empty gesture packets for
invalid
> > events, and dispatch the invalid events as usual?
> > 
> > The specific issue I'm running into is related to the difference between
> > ET_TOUCH_CANCELLED and ACTION_CANCEL.
> > The gesture detector, upon seeing an ACTION_CANCEL, assumes all touches have
> > been released, and then considers future ACTION_CANCEL events invalid.
> > 
> > In the GestureProviderAura, I'm currently converting all ET_TOUCH_CANCELLED
> > events into ACTION_CANCEL MotionEvents. This works fine without the eager
GR,
> > because we never cancel some touch events without cancelling all of the
touch
> > events. 
> > 
> 
> Who synthesizes, and when, these ET_TOUCH_CANCELLED events? Will it ever
happen
> that one pointer is cancelled but not another? If so, then yes we have a
problem
> and might need to modify the gesture detector to accommodate this. 

There are a bunch of cases where we cancel all the pointers. We never synthesize
cancels for some pointers but not others.
- A window gains capture (i.e., you're touching, and a javascript alert fires)
- We need to transfer the touch stream (drag and drop)
- The device orientation changes

> Otherwise,
> perhaps we could adjust the semantics of ET_TOUCH_CANCELLED so that it's a
> broadcast for all active pointers?

That would require modifying a bunch of aura components which currently listen
for multiple ET_TOUCH_CANCELLED events. I'm not sure how big a pain this would
be, but it would definitely be a pain. It also doesn't fit in very well with the
aura touch event model. Every touch event carries information about a single
touch point. I'm not too keen on changing that.

> 
> > Without modifying the gesture detector, the eager GR will now prevent us
from
> > dispatching multiple touch cancel events, which will potentially break some
> > things in Aura.
> > 
> > It would be possible to fix this specific issue, but even in general, it
feels
> > somewhat strange to allow the gesture detector to prevent touch events from
> > being dispatched.
> 
> The GR doesn't really care what events get forwarded, it's the eager GR.  The
> original motivation for conditionally forwarding events was preventing a
partial
> touch sequence from being forwarded, giving strong guarantees on "complete"
> event stream delivery.  

In what cases are we seeing incomplete event streams on Android?

I'm worried that this will be a significant behavior change for Aura which will
cause us a bunch of unnecessary hassle.

I'm also not sure that the GR is the right place to enforce those guarantees. In
a (nearly) perfect world, we'd sanitize the touch input as soon as we got it,
and then ASSERT that it's valid throughout the rest of the stack. Obviously we
aren't there yet, so maybe the GR is a reasonable place for this sanitization.

> If those events aren't forwarded, the eager GR won't
> expect an ack, so it drops them entirely.  I suppose we could change this
> behavior, but it would be a shame to do it solely for the case of
ACTION_CANCEL?

I agree that we definitely don't want to change this behavior for Android solely
because of ACTION_CANCEL behavior on Aura.

None of the options that I've thought of seem very pleasant to me:
1. Aura doesn't let the GR filter out touches 
   - This would require some hack like adding empty gesture packets for invalid
touches
2. The GestureDetector behaves differently on Aura, and cancels touches one at a
time
3. Change the meaning of ET_TOUCH_CANCELLED, and update all affected consumers

Do you have any better ideas?

[jdduke (slow), 2014-07-23 16:02:09]

On 2014/07/23 15:54:41, tdresser wrote:
> Do you have any better ideas?

For now, what if we make an exception for ACTION_CANCEL whereby we allow it to
be forwarded even if the GR doesn't have an active ACTION_DOWN event? I don't
think there's much harm in doing that, certainly not from a perf perspective as
the touches should get filtered by the TouchEventQueue anyhow. It's also not a
relatively frequent event... wdyt? The change could be made either in
GestureProvider or FilteredGestureProvider.

[tdresser, 2014-07-28 18:22:10]

I'm moved the necessary changes to the touch exploration code to
https://codereview.chromium.org/428673002. I'll land it first.

This is a little strange, as we're now using ACTION_CANCEL to mean two very
different things under Aura and Android, but it appears to work fine.

It means that some classes of touch streams which previously would not have been
ignored will not be ignored, but it hasn't broken any of the tricky cases that
I've tested (tab dragging, drag and drop).

Sadrul, can you take another look?
Jared, does this look sane to you?

https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/393953012/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1923:
window_->GetHost()->dispatcher()->OnForwardingTouchEvent(event, window_);
On 2014/07/16 21:07:48, sadrul wrote:
> Can you explain what OnForwardingTouchEvent does, and why this is necessary?
> (e.g. the code in WindowEventDispatcher::PostDispatchEvent() should be
> sufficient, except it would ignore event.result())

Moved to PreDispatchEvent.

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:517: }
On 2014/07/16 21:07:49, sadrul wrote:
> Should this be something like:
>   if (unified && !consumed) {
>     gestures_.reset(AckTouchEventForGesture(....));
>   else
>     gestures_.reset(ProcessTouchEventForGesture(....));
> 
> to avoid creating gestures in line 508, and then discarding them in line 514?

This issue has been addressed in the most recent patch.

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/393953012/diff/40001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.h:96: // Called before the event is forwarded to
the |host_|.
On 2014/07/16 21:07:49, sadrul wrote:
> Is this comment correct?

OnForwardingTouchEvent is now gone.

[tdresser, 2014-07-30 13:52:36]

Jared, does this seem like a reasonable approach to you?

[jdduke (slow), 2014-07-30 15:29:32]

https://codereview.chromium.org/393953012/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/393953012/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:120: } else {
This branching is unfortunate but I guess it won't be around much longer?

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_aura.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:71: if (screen)
This is scary, when will this be null? Should we be asserting here?

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:793:
Send(CreateGesture(ET_GESTURE_END,
Can this just be |CreateGesture(ET_GESTURE_END, event)|? Then you can also get
rid of the manual GetBoundingBoxCreation which doesn't seem necessary when
|gesture_begin_end_types_enabled_| is false?

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:39: event.force() ==
pointer_state_.GetPressure(i)) {
How noisy are force/radius values compared to position? I'd rather we preserve
the logic we had in ui_events_helper.cc for now (only comparing the position),
and fold in the radius/pressure if we find that necessary in the future.

[tdresser, 2014-07-31 15:33:32]

https://codereview.chromium.org/393953012/diff/260001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/393953012/diff/260001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:120: } else {
On 2014/07/30 15:29:32, jdduke wrote:
> This branching is unfortunate but I guess it won't be around much longer?

Assuming we can land eager GR in 38, I'm hoping to clean this up soon after the
M38 branch.

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_aura.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:71: if (screen)
On 2014/07/30 15:29:32, jdduke wrote:
> This is scary, when will this be null? Should we be asserting here?

Added a comment indicating that |screen| is sometimes null during tests.

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:793:
Send(CreateGesture(ET_GESTURE_END,
On 2014/07/30 15:29:32, jdduke wrote:
> Can this just be |CreateGesture(ET_GESTURE_END, event)|? Then you can also get
> rid of the manual GetBoundingBoxCreation which doesn't seem necessary when
> |gesture_begin_end_types_enabled_| is false? 

Thanks. Done.

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:39: event.force() ==
pointer_state_.GetPressure(i)) {
On 2014/07/30 15:29:32, jdduke wrote:
> How noisy are force/radius values compared to position? I'd rather we preserve
> the logic we had in ui_events_helper.cc for now (only comparing the position),
> and fold in the radius/pressure if we find that necessary in the future.

The amount of noise depends entirely on the firmware configuration, but you're
right that there is frequently more noise in the radius and pressure.

It feels wrong to me to be artificially filtering out legitimate changes in
radius and pressure though. If we feel the signal is too noisy, maybe we should
be smoothing it?

That being said, I don't care particularly strongly about it, if you'd prefer
only looking at location, that's fine.

[jdduke (slow), 2014-07-31 16:19:03]

https://codereview.chromium.org/393953012/diff/260001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:166: if
(ui::IsUnifiedGestureDetectorEnabled()) {
Would it be crazy to abstract all of these differences into the body of
GestureRecognizer? So then we'd have two entry points:

ProcessTouchEventBeforeDispatch();
ProcessTouchEventAfterDispatch();

and internally it would do the right thing depending on whether the unified GR
is enabled?

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:39: event.force() ==
pointer_state_.GetPressure(i)) {
On 2014/07/31 15:33:31, tdresser wrote:
> On 2014/07/30 15:29:32, jdduke wrote:
> > How noisy are force/radius values compared to position? I'd rather we
preserve
> > the logic we had in ui_events_helper.cc for now (only comparing the
position),
> > and fold in the radius/pressure if we find that necessary in the future.
> 
> The amount of noise depends entirely on the firmware configuration, but you're
> right that there is frequently more noise in the radius and pressure.
> 
> It feels wrong to me to be artificially filtering out legitimate changes in
> radius and pressure though. If we feel the signal is too noisy, maybe we
should
> be smoothing it?
> 
> That being said, I don't care particularly strongly about it, if you'd prefer
> only looking at location, that's fine.

It's not that I feel strongly about it, more so that this patch introduces a
substantial behavior change, so the fewer additional differences we layer in,
the better.  We can always fix it in a follow-up patch, and you're probably
right that using the size here makes sense.

https://codereview.chromium.org/393953012/diff/280001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_config_helper_aura.cc (right):

https://codereview.chromium.org/393953012/diff/280001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:71: // |screen| is
sometimes NULL during tests.
Hmm, I wonder if it would be an improvement to have Display be an argument to
this function? Maybe some other day... (actually I'd like to get rid of the
Display dependency entirely but that's for another day...).

[tdresser, 2014-08-01 13:35:32]

https://codereview.chromium.org/393953012/diff/260001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:166: if
(ui::IsUnifiedGestureDetectorEnabled()) {
On 2014/07/31 16:19:02, jdduke wrote:
> Would it be crazy to abstract all of these differences into the body of
> GestureRecognizer? So then we'd have two entry points:
> 
> ProcessTouchEventBeforeDispatch();
> ProcessTouchEventAfterDispatch();
> 
> and internally it would do the right thing depending on whether the unified GR
> is enabled?

Done.

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/393953012/diff/260001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:39: event.force() ==
pointer_state_.GetPressure(i)) {
On 2014/07/31 16:19:02, jdduke wrote:
> On 2014/07/31 15:33:31, tdresser wrote:
> > On 2014/07/30 15:29:32, jdduke wrote:
> > > How noisy are force/radius values compared to position? I'd rather we
> preserve
> > > the logic we had in ui_events_helper.cc for now (only comparing the
> position),
> > > and fold in the radius/pressure if we find that necessary in the future.
> > 
> > The amount of noise depends entirely on the firmware configuration, but
you're
> > right that there is frequently more noise in the radius and pressure.
> > 
> > It feels wrong to me to be artificially filtering out legitimate changes in
> > radius and pressure though. If we feel the signal is too noisy, maybe we
> should
> > be smoothing it?
> > 
> > That being said, I don't care particularly strongly about it, if you'd
prefer
> > only looking at location, that's fine.
> 
> It's not that I feel strongly about it, more so that this patch introduces a
> substantial behavior change, so the fewer additional differences we layer in,
> the better.  We can always fix it in a follow-up patch, and you're probably
> right that using the size here makes sense.

Done.

[jdduke (slow), 2014-08-01 14:27:22]

content/browser/renderer_host/input and ui/events/gesture_detection lgtm.

https://codereview.chromium.org/393953012/diff/300001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/393953012/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:113: this)) {
I can't tell if this line break is aligned?

https://codereview.chromium.org/393953012/diff/300001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/300001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:27: #include
"ui/events/gestures/unified_gesture_detector_enabled.h"
Looks like we don't need this include anymore?

https://codereview.chromium.org/393953012/diff/300001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/393953012/diff/300001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer.h:32: virtual Gestures*
ProcessTouchEventPostDispatch(const TouchEvent& event,
This is great! Probably worth keeping the comment about the caller's
responsibility for freeing |Gestures*| (and the semantic difference between
PostDispatch and OnAsyncAck).  Thanks!

[tdresser, 2014-08-01 18:54:00]

https://codereview.chromium.org/393953012/diff/300001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/393953012/diff/300001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.cc:113: this)) {
On 2014/08/01 14:27:22, jdduke wrote:
> I can't tell if this line break is aligned?

Nice catch, thanks.
Done.

https://codereview.chromium.org/393953012/diff/300001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/300001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:27: #include
"ui/events/gestures/unified_gesture_detector_enabled.h"
On 2014/08/01 14:27:22, jdduke wrote:
> Looks like we don't need this include anymore?

Done.

https://codereview.chromium.org/393953012/diff/300001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/393953012/diff/300001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer.h:32: virtual Gestures*
ProcessTouchEventPostDispatch(const TouchEvent& event,
On 2014/08/01 14:27:22, jdduke wrote:
> This is great! Probably worth keeping the comment about the caller's
> responsibility for freeing |Gestures*| (and the semantic difference between
> PostDispatch and OnAsyncAck).  Thanks!

Done.

[sadrul, 2014-08-01 21:00:06]

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:166: // Once we've fully migrated to the
unified gesture detector, we won't need to
Do you mean 'eager' instead of 'unified'?

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:498: // Once we've fully migrated to the
unified gesture detector, we won't
ditto

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:840: DispatchEventUsingWindowDispatcher(&press2);
This seems odd. Why is it necessary to dispatch a second press at the same
location?

[tdresser, 2014-08-05 14:40:14]

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:166: // Once we've fully migrated to the
unified gesture detector, we won't need to
On 2014/08/01 21:00:06, sadrul wrote:
> Do you mean 'eager' instead of 'unified'?

Done.

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:498: // Once we've fully migrated to the
unified gesture detector, we won't
On 2014/08/01 21:00:06, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/393953012/diff/320001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:840: DispatchEventUsingWindowDispatcher(&press2);
On 2014/08/01 21:00:06, sadrul wrote:
> This seems odd. Why is it necessary to dispatch a second press at the same
> location?

No longer doing this.

[sadrul, 2014-08-06 00:46:09]

LGTM

https://codereview.chromium.org/393953012/diff/360001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/360001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:164: static_cast<Window*>(window),
Don't need the casts here.

https://codereview.chromium.org/393953012/diff/360001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:885: static_cast<const
ui::TouchEvent&>(*event), target, window());
Don't need the cast

https://codereview.chromium.org/393953012/diff/360001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer_impl.cc (right):

https://codereview.chromium.org/393953012/diff/360001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:294: SetupTargets(event,
consumer);
I think this is a slight change in behaviour: we currently set the target after
the touch-event has been dispatched. With this change, we set the target before
the touch-event has been dispatched. This can break if, for example, there are
nested event-dispatch. I can't think of an actual case where this may be a
problem, but we should keep any eye on this for incoming bugs.

[tdresser, 2014-08-06 12:47:04]

jochen@, can you take a look at:

content/browser/frame_host/render_widget_host_view_guest.cc
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
content/public/test/browser_test_utils.cc

https://codereview.chromium.org/393953012/diff/360001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/393953012/diff/360001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:164: static_cast<Window*>(window),
On 2014/08/06 00:46:08, sadrul wrote:
> Don't need the casts here.

Done.

https://codereview.chromium.org/393953012/diff/360001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:885: static_cast<const
ui::TouchEvent&>(*event), target, window());
On 2014/08/06 00:46:08, sadrul wrote:
> Don't need the cast

Done.

https://codereview.chromium.org/393953012/diff/360001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer_impl.cc (right):

https://codereview.chromium.org/393953012/diff/360001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:294: SetupTargets(event,
consumer);
On 2014/08/06 00:46:08, sadrul wrote:
> I think this is a slight change in behaviour: we currently set the target
after
> the touch-event has been dispatched. With this change, we set the target
before
> the touch-event has been dispatched. This can break if, for example, there are
> nested event-dispatch. I can't think of an actual case where this may be a
> problem, but we should keep any eye on this for incoming bugs.

Hmmm, good thought. I'll keep it in mind.

[jochen (gone - plz use gerrit), 2014-08-07 14:59:37]

lgtm

[tdresser, 2014-08-07 15:21:58]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-08-07 15:23:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/393953012/380001

[commit-bot: I haz the power, 2014-08-07 15:35:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38576)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43778)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-07 15:36:44]

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

[commit-bot: I haz the power, 2014-08-07 15:36:45]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38581)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tdresser, 2014-08-07 17:45:10]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-08-07 17:56:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/393953012/400001

[commit-bot: I haz the power, 2014-08-07 19:38:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 00:59:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 06:46:37]

Change committed as 288236