Re-enable Eager Gesture Recognition on Aura

Closed - replaced by https://codereview.chromium.org/680413006

Re-enable Eager Gesture Recognition on Aura

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

The previous attempt at enabling eager gesture recognition
(crrev.com/393953012) failed.
See crbug.com/408594 for details.

BUG=332418
TEST=GestureRecognizerTest.{*,EagerGestureDetection}

[tdresser, 2014-10-22 14:15:11]

tdresser@chromium.org changed reviewers:
+ jdduke@chromium.org

[tdresser, 2014-10-22 14:18:53]

On 2014/10/22 14:15:11, tdresser wrote:
> mailto:tdresser@chromium.org changed reviewers:
> + mailto:jdduke@chromium.org

I've done thorough manual testing on Chrome OS, I still need to do manual
testing on Windows.

This feels pretty solid to me though.
It's a bit of a shame to give the WindowDelegate this much more responsibility,
but I think its the best way forward.

[tdresser, 2014-10-22 17:45:00]

On 2014/10/22 14:18:53, tdresser wrote:
> On 2014/10/22 14:15:11, tdresser wrote:
> > mailto:tdresser@chromium.org changed reviewers:
> > + mailto:jdduke@chromium.org
> 
> I've done thorough manual testing on Chrome OS, I still need to do manual
> testing on Windows.
> 
> This feels pretty solid to me though.
> It's a bit of a shame to give the WindowDelegate this much more
responsibility,
> but I think its the best way forward.

Looks good on Windows too.

[jdduke (slow), 2014-10-22 17:51:02]

The window_event_dispatcher.cc is a bit outside my domain, but the rest seems
reasonable. Will sadrul@ be back soon or should we push this forward?

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

https://codereview.chromium.org/551373006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2049:
event->StopPropagation();
Does it matter that the touch may have been ack'ed synchronously even if it was
forwarded? Will calling StopPropagation do the right thing in that case?

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
ui/events/gestures/gesture_recognizer.h:35: // freeing the returned events.
Called synchronously after event dispatch.
Is the "synchronously" comment here still always valid?

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_recognizer_impl.h (right):

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
ui/events/gestures/gesture_recognizer_impl.h:62: virtual bool
ProcessTouchEvent(const TouchEvent& event,
Nit: No need for virtual with override any longer (and below). I guess the reset
of this class hasn't adopted that yet... change up to you.

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

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

[tdresser, 2014-10-22 18:24:08]

He's back!

sadrul@, can you take a look at 
ui/aura
content (I know you aren't an owner of everything, I'll grab someone else for
OWNERs review later)

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

https://codereview.chromium.org/551373006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2049:
event->StopPropagation();
On 2014/10/22 17:51:02, jdduke wrote:
> Does it matter that the touch may have been ack'ed synchronously even if it
was
> forwarded? Will calling StopPropagation do the right thing in that case?

In that case, we'll synchronously hit
WindowEventDispatcher::ProcessedTouchEvent, and turn the touch into a gesture
(if necessary), before the StopPropagation call occurs. 

I think its fine.

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
ui/events/gestures/gesture_recognizer.h:35: // freeing the returned events.
Called synchronously after event dispatch.
On 2014/10/22 17:51:02, jdduke wrote:
> Is the "synchronously" comment here still always valid?

Absolutely not. Fixed.

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_recognizer_impl.h (right):

https://codereview.chromium.org/551373006/diff/60001/ui/events/gestures/gestu...
ui/events/gestures/gesture_recognizer_impl.h:62: virtual bool
ProcessTouchEvent(const TouchEvent& event,
On 2014/10/22 17:51:02, jdduke wrote:
> Nit: No need for virtual with override any longer (and below). I guess the
reset
> of this class hasn't adopted that yet... change up to you.

Done.

[sadrul, 2014-10-23 15:56:34]

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

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:163: // pass an event here.
Is this comment still correct?

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:494: if(event.result() & ui::ER_CONSUMED)
space after 'if'.

I remember we discussed using some other mechanism for detecting whether an
event is being handled async. I don't remember what we concluded from that
discussion. Can you remind me?

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:505: orig_event,
static_cast<Window*>(target))) {
As the TODO you are removing mentions, shouldn't this move to PreDispatch phase?

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

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.h:125: virtual bool
OnForwardingAsyncTouchEvent(ui::TouchEvent* event);
why virtual?

'Forwarding' here doesn't really make a lot of sense. Can we call this something
like 'CanProcessTouchEventAsync' instead?

https://codereview.chromium.org/551373006/diff/80001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/551373006/diff/80001/ui/events/gestures/gestu...
ui/events/gestures/gesture_recognizer.h:33: GestureConsumer* consumer) = 0;
Fix indent

[tdresser, 2014-10-27 15:52:32]

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

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:163: // pass an event here.
On 2014/10/23 15:56:34, sadrul wrote:
> Is this comment still correct?

Yes, this is still correct.
This patch only migrates to using eager gesture recognition, it doesn't perform
any of the related cleanup.

The cleanup (very out of date) is here:
https://codereview.chromium.org/510793003/
https://codereview.chromium.org/503883004/

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:494: if(event.result() & ui::ER_CONSUMED)
On 2014/10/23 15:56:34, sadrul wrote:
> space after 'if'.
> 
> I remember we discussed using some other mechanism for detecting whether an
> event is being handled async. I don't remember what we concluded from that
> discussion. Can you remind me?

(Added space after 'if')

We concluded that it was a perfectly reasonable approach. I then implemented it,
and ended up with a bunch of subtle bugs. The bugs weren't necessarily related
to using a different mechanism for detecting if an event was being handled
async, but they did necessitate taking a different high level approach.

In this new approach, it isn't necessary to use a different mechanism for
detecting if an event is being handled async. I thought that since it isn't
necessary, it doesn't make sense to do in this patch. If we think it would be a
worthwhile change, we can always modify it later.

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:505: orig_event,
static_cast<Window*>(target))) {
On 2014/10/23 15:56:34, sadrul wrote:
> As the TODO you are removing mentions, shouldn't this move to PreDispatch
phase?

I previously thought that was the correct path forward, but it turned out to be
trickier than I anticipated. The hard part is ensuring that every event passed
to ProcessTouchEvent is also passed to ProcessTouchEventAck. The best way to do
that is to put both in PostDispatchEvent for the synchronous case, which has the
somewhat unfortunate side effect of giving more responsibility to the
RenderWidgetHostViewAura.

I think this is the best approach though.

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

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.h:125: virtual bool
OnForwardingAsyncTouchEvent(ui::TouchEvent* event);
On 2014/10/23 15:56:34, sadrul wrote:
> why virtual?
Done.
> 
> 'Forwarding' here doesn't really make a lot of sense. Can we call this
something
> like 'CanProcessTouchEventAsync' instead?

CanProcessTouchEventAsync makes it sound side-effect free, which it definitely
isn't.

How about 'OnProcessingTouchEventAsync'?

https://codereview.chromium.org/551373006/diff/80001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/551373006/diff/80001/ui/events/gestures/gestu...
ui/events/gestures/gesture_recognizer.h:33: GestureConsumer* consumer) = 0;
On 2014/10/23 15:56:34, sadrul wrote:
> Fix indent

Done.

[tdresser, 2014-10-28 15:25:53]

Patchset #7 (id:120001) has been deleted

[tdresser, 2014-10-29 14:57:09]

On 2014/10/27 15:52:32, tdresser wrote:
>
https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
> File ui/aura/window_event_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
> ui/aura/window_event_dispatcher.cc:163: // pass an event here.
> On 2014/10/23 15:56:34, sadrul wrote:
> > Is this comment still correct?
> 
> Yes, this is still correct.
> This patch only migrates to using eager gesture recognition, it doesn't
perform
> any of the related cleanup.
> 
> The cleanup (very out of date) is here:
> https://codereview.chromium.org/510793003/
> https://codereview.chromium.org/503883004/
> 
>
https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
> ui/aura/window_event_dispatcher.cc:494: if(event.result() & ui::ER_CONSUMED)
> On 2014/10/23 15:56:34, sadrul wrote:
> > space after 'if'.
> > 
> > I remember we discussed using some other mechanism for detecting whether an
> > event is being handled async. I don't remember what we concluded from that
> > discussion. Can you remind me?
> 
> (Added space after 'if')
> 
> We concluded that it was a perfectly reasonable approach. I then implemented
it,
> and ended up with a bunch of subtle bugs. The bugs weren't necessarily related
> to using a different mechanism for detecting if an event was being handled
> async, but they did necessitate taking a different high level approach.
> 
> In this new approach, it isn't necessary to use a different mechanism for
> detecting if an event is being handled async. I thought that since it isn't
> necessary, it doesn't make sense to do in this patch. If we think it would be
a
> worthwhile change, we can always modify it later.
> 
>
https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
> ui/aura/window_event_dispatcher.cc:505: orig_event,
> static_cast<Window*>(target))) {
> On 2014/10/23 15:56:34, sadrul wrote:
> > As the TODO you are removing mentions, shouldn't this move to PreDispatch
> phase?
> 
> I previously thought that was the correct path forward, but it turned out to
be
> trickier than I anticipated. The hard part is ensuring that every event passed
> to ProcessTouchEvent is also passed to ProcessTouchEventAck. The best way to
do
> that is to put both in PostDispatchEvent for the synchronous case, which has
the
> somewhat unfortunate side effect of giving more responsibility to the
> RenderWidgetHostViewAura.
> 
> I think this is the best approach though.
> 
>
https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
> File ui/aura/window_event_dispatcher.h (right):
> 
>
https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
> ui/aura/window_event_dispatcher.h:125: virtual bool
> OnForwardingAsyncTouchEvent(ui::TouchEvent* event);
> On 2014/10/23 15:56:34, sadrul wrote:
> > why virtual?
> Done.
> > 
> > 'Forwarding' here doesn't really make a lot of sense. Can we call this
> something
> > like 'CanProcessTouchEventAsync' instead?
> 
> CanProcessTouchEventAsync makes it sound side-effect free, which it definitely
> isn't.
> 
> How about 'OnProcessingTouchEventAsync'?
> 
>
https://codereview.chromium.org/551373006/diff/80001/ui/events/gestures/gestu...
> File ui/events/gestures/gesture_recognizer.h (right):
> 
>
https://codereview.chromium.org/551373006/diff/80001/ui/events/gestures/gestu...
> ui/events/gestures/gesture_recognizer.h:33: GestureConsumer* consumer) = 0;
> On 2014/10/23 15:56:34, sadrul wrote:
> > Fix indent
> 
> Done.

Ping.

[sadrul, 2014-10-29 19:35:00]

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

https://codereview.chromium.org/551373006/diff/80001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:505: orig_event,
static_cast<Window*>(target))) {
On 2014/10/27 15:52:31, tdresser wrote:
> On 2014/10/23 15:56:34, sadrul wrote:
> > As the TODO you are removing mentions, shouldn't this move to PreDispatch
> phase?
> 
> I previously thought that was the correct path forward, but it turned out to
be
> trickier than I anticipated. The hard part is ensuring that every event passed
> to ProcessTouchEvent is also passed to ProcessTouchEventAck. The best way to
do
> that is to put both in PostDispatchEvent for the synchronous case, which has
the
> somewhat unfortunate side effect of giving more responsibility to the
> RenderWidgetHostViewAura.
> 
> I think this is the best approach though.

I thought one of the major points of eager GR was to do the gesture-recognition
before the touch-event is processed (i.e. dispatched). So it's odd that we are
doing this after the touch event has already been processed.

It looks like we always call GR::ProcessTouchEvent() during/after dispatch, and
then either immediately call GR::ProcessTouchEventAck(), or call it
asynchronously. Is this right? If so, can we do:

  WED receives touch event:
    GR::ProcessTouchEvent
    if continue:
      dispatch touch event
      if touch-event ER_CONSUMED: do nothing for now.
      otherwise, GR::ProcessTouchEventAck.


The RWHVA can then call into WED::ProcessedTouchEvent() when it receives the
ack, and we won't need WED::OnProcessingTouchEventAsync(). Would that work?