Re-enable Eager Gesture Recognition on Aura

ui/aura/window_event_dispatcher.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/aura/window_event_dispatcher.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 141 matching lines @@
 DispatchDetails WindowEventDispatcher::DispatchMouseExitAtPoint(
     const gfx::Point& point) {
   ui::MouseEvent event(ui::ET_MOUSE_EXITED, point, point, ui::EF_NONE,
                        ui::EF_NONE);
   return DispatchMouseEnterOrExit(event, ui::ET_MOUSE_EXITED);
 }
 
 void WindowEventDispatcher::ProcessedTouchEvent(ui::TouchEvent* event,
                                                 Window* window,
                                                 ui::EventResult result) {
-  // TODO(tdresser): Move this to PreDispatchTouchEvent, to enable eager
-  // gesture detection. See crbug.com/410280.
-  if (!ui::GestureRecognizer::Get()
-           ->ProcessTouchEventPreDispatch(*event, window)) {
-    return;
-  }
-
   // Once we've fully migrated to the eager gesture detector, we won't need to
   // pass an event here.

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

Is this comment still correct?

[tdresser, 2014/10/27 15:52:31]

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/

   scoped_ptr<ui::GestureRecognizer::Gestures> gestures(
-      ui::GestureRecognizer::Get()->ProcessTouchEventOnAsyncAck(
-          *event, result, window));
+      ui::GestureRecognizer::Get()
+          ->ProcessTouchEventAck(*event, result, window));
   DispatchDetails details = ProcessGestures(gestures.get());
   if (details.dispatcher_destroyed)
     return;
 }
 
 void WindowEventDispatcher::HoldPointerMoves() {
   if (!move_hold_count_)
     held_event_factory_.InvalidateWeakPtrs();
   ++move_hold_count_;
   TRACE_EVENT_ASYNC_BEGIN0("ui", "WindowEventDispatcher::HoldPointerMoves",
@@ skipping 306 matching lines @@
   old_dispatch_target_ = NULL;
 #ifndef NDEBUG
   DCHECK(!event_dispatch_target_ || window()->Contains(event_dispatch_target_));
 #endif
 
   if (event.IsTouchEvent() && !details.target_destroyed) {
     // Do not let 'held' touch events contribute to any gestures unless it is
     // being dispatched.
     if (dispatching_held_event_ || !held_move_event_ ||
         !held_move_event_->IsTouchEvent()) {
+      // If the event is being handled asynchronously, ignore it.
+      if(event.result() & ui::ER_CONSUMED)

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

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?

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

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

+        return details;
+      scoped_ptr<ui::GestureRecognizer::Gestures> gestures;
 
       // Once we've fully migrated to the eager gesture detector, we won't
       // need to pass an event here.
       ui::TouchEvent orig_event(static_cast<const ui::TouchEvent&>(event),
                                 static_cast<Window*>(event.target()),
                                 window());
 
-      if (event.result() & ui::ER_CONSUMED)
-        orig_event.StopPropagation();
-
-      // TODO(tdresser): Move this to PreDispatchTouchEvent, to enable eager
-      // gesture detection. See crbug.com/410280.
-      if (!ui::GestureRecognizer::Get()
-               ->ProcessTouchEventPreDispatch(orig_event,
-                                              static_cast<Window*>(target))) {
-        return details;
+      if (ui::GestureRecognizer::Get()->ProcessTouchEvent(
+              orig_event, static_cast<Window*>(target))) {

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

As the TODO you are removing mentions, shouldn't this move to PreDispatch phase?

[tdresser, 2014/10/27 15:52:31]

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.

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

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?

+        gestures.reset(ui::GestureRecognizer::Get()->ProcessTouchEventAck(
+            orig_event, event.result(), static_cast<Window*>(target)));
+        return ProcessGestures(gestures.get());
       }
-
-      scoped_ptr<ui::GestureRecognizer::Gestures> gestures;
-
-      gestures.reset(
-          ui::GestureRecognizer::Get()->ProcessTouchEventPostDispatch(
-              orig_event, event.result(), static_cast<Window*>(target)));
-
-      return ProcessGestures(gestures.get());
     }
   }
 
   return details;
 }
 
+bool WindowEventDispatcher::OnForwardingAsyncTouchEvent(ui::TouchEvent* event) {
+  ui::TouchEvent orig_event(
+      *event, static_cast<aura::Window*>(event->target()), window());
+
+  bool should_forward = ui::GestureRecognizer::Get()->ProcessTouchEvent(
+      orig_event, static_cast<aura::Window*>(event->target()));
+
+  // Make sure that |PostDispatch| doesn't give |event| to the GestureRecognizer
+  // again.
+  event->StopPropagation();
+  return should_forward;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // WindowEventDispatcher, ui::GestureEventHelper implementation:
 
 bool WindowEventDispatcher::CanDispatchToConsumer(
     ui::GestureConsumer* consumer) {
   Window* consumer_window = ConsumerToWindow(consumer);
   return (consumer_window && consumer_window->GetRootWindow() == window());
 }
 
 void WindowEventDispatcher::DispatchCancelTouchEvent(ui::TouchEvent* event) {
@@ skipping 356 matching lines @@
 
     default:
       NOTREACHED();
       break;
   }
 
   PreDispatchLocatedEvent(target, event);
 }
 
 }  // namespace aura