Condense RootView::DispatchGestureEvent() to a single loop

ui/views/widget/root_view.cc

 // Copyright (c) 2012 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/views/widget/root_view.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 232 matching lines @@
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RootView, ui::EventProcessor overrides:
 
 ui::EventTarget* RootView::GetRootTarget() {
   return this;
 }
 
 ui::EventDispatchDetails RootView::OnEventFromSource(ui::Event* event) {
-  // TODO(tdanderson): Replace the calls to Dispatch*Event() with calls to
-  //                   EventProcessor::OnEventFromSource() once the code for
-  //                   that event type has been refactored, and then
-  //                   eventually remove this function altogether. See
-  //                   crbug.com/348083.
-
   if (event->IsKeyEvent())
     return EventProcessor::OnEventFromSource(event);
 
   if (event->IsScrollEvent())
     return EventProcessor::OnEventFromSource(event);
 
   if (event->IsGestureEvent()) {
+    // TODO(tdanderson): Once DispatchGestureEvent() has been removed, move
+    //                   all of this logic into an override of a new
+    //                   virtual method
+    //                   EventProcessor::OnEventProcessingStarted() (which
+    //                   returns false if no processing should take place).
+    //                   Also move the implementation of
+    //                   PrepareEventForDispatch() into this new method.
+    //                   Then RootView::OnEventFromSource() can be removed.
     ui::GestureEvent* gesture_event = event->AsGestureEvent();
 
     // Do not dispatch ui::ET_GESTURE_BEGIN events.
     if (gesture_event->type() == ui::ET_GESTURE_BEGIN)
       return DispatchDetails();
 
     // Ignore ui::ET_GESTURE_END events which do not correspond to the
     // removal of the final touch point.
     if (gesture_event->type() == ui::ET_GESTURE_END &&
         gesture_event->details().touch_points() > 1) {
@@ skipping 361 matching lines @@
 View::DragInfo* RootView::GetDragInfo() {
   return &drag_info_;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RootView, private:
 
 // Input -----------------------------------------------------------------------
 
 void RootView::DispatchGestureEvent(ui::GestureEvent* event) {
-  if (gesture_handler_) {
-    if (gesture_handler_->enabled()) {
-      // |gesture_handler_| can be deleted during processing. In particular, it
-      // will be set to NULL if the view is deleted or removed from the tree as
-      // a result of an event dispatch.
-      ui::GestureEvent handler_event(*event,
-                                     static_cast<View*>(this),
-                                     gesture_handler_);
-      ui::EventDispatchDetails dispatch_details =
-          DispatchEvent(gesture_handler_, &handler_event);
-      if (dispatch_details.dispatcher_destroyed)
-        return;
-
-      if (handler_event.stopped_propagation())
-        event->StopPropagation();
-      else if (handler_event.handled())
-        event->SetHandled();
-    } else {
-      // Disabled views are permitted to be targets of gesture events, but
-      // gesture events should never actually be dispatched to them.
-      event->SetHandled();
-    }
-
-    if (event->type() == ui::ET_GESTURE_END) {
-      DCHECK_EQ(1, event->details().touch_points());
-      // In case a drag was in progress, reset all the handlers. Otherwise, just
-      // reset the gesture handler.
-      if (gesture_handler_ == mouse_pressed_handler_)
-        SetMouseHandler(NULL);
-      else
-        gesture_handler_ = NULL;
-    }
-
-    if (event->handled())
-      return;
-
-    if (event->type() == ui::ET_GESTURE_SCROLL_BEGIN) {
-      // Some view started processing gesture events, however it does not
-      // process scroll-gesture events. In such case, we allow the event to
-      // bubble up. |gesture_handler_| is changed to its nearest ancestor
-      // that handles scroll-gesture events.
-      gesture_handler_ = static_cast<View*>(
-          targeter()->FindNextBestTarget(gesture_handler_, event));
-      while (gesture_handler_ && gesture_handler_ != this) {
-        ui::GestureEvent gesture_event(*event,
-                                       static_cast<View*>(this),
-                                       gesture_handler_);
-        ui::EventDispatchDetails dispatch_details =
-            DispatchEvent(gesture_handler_, &gesture_event);
-        if (gesture_event.stopped_propagation()) {
-          event->StopPropagation();
-          return;
-        } else if (gesture_event.handled()) {
-          event->SetHandled();
-          return;
-        } else if (dispatch_details.dispatcher_destroyed ||
-                   dispatch_details.target_destroyed) {
-          return;
-        }
-        gesture_handler_ = static_cast<View*>(
-            targeter()->FindNextBestTarget(gesture_handler_, event));
-      }
-      gesture_handler_ = NULL;
-    }
-
-    return;
-  }
-
-  // Walk up the tree until we find a view that wants the gesture event.
-  gesture_handler_ =
+  bool gesture_handler_set_before_dispatch = !!gesture_handler_;
+  View* target =
       static_cast<View*>(targeter()->FindTargetForEvent(this, event));
-  while (gesture_handler_ && gesture_handler_ != this) {
-    // Disabled views are permitted to be targets of gesture events, but
-    // gesture events should never actually be dispatched to them.
-    if (!gesture_handler_->enabled()) {
-      event->SetHandled();
-
-      // Last ui::ET_GESTURE_END should not set the gesture_handler_.
-      if (event->type() == ui::ET_GESTURE_END) {
-        DCHECK_EQ(1, event->details().touch_points());
-        gesture_handler_ = NULL;
-      }
-
-      return;
-    }
-
-    // See if this view wants to handle the Gesture.
-    ui::GestureEvent gesture_event(*event,
-                                   static_cast<View*>(this),
-                                   gesture_handler_);
+  while (target && target != this) {
+    // Create and dispatch a copy of |event|.
+    ui::GestureEvent event_copy(*event, static_cast<View*>(this), target);
     ui::EventDispatchDetails dispatch_details =
-        DispatchEvent(gesture_handler_, &gesture_event);
+        DispatchEvent(target, &event_copy);
     if (dispatch_details.dispatcher_destroyed)
       return;
 
-    // The view could have removed itself from the tree when handling
-    // OnGestureEvent(). So handle as per OnMousePressed. NB: we
-    // assume that the RootView itself cannot be so removed.
-    if (!gesture_handler_)
+    if (event_copy.stopped_propagation())
+      event->StopPropagation();
+    else if (event_copy.handled())
+      event->SetHandled();
+
+    // If the event was handled by the previous dispatch or if the target
+    // was destroyed, do not allow any further processing of |event|.
+    if (event->handled() || dispatch_details.target_destroyed)
       return;
 
-    if (gesture_event.handled()) {
-      if (gesture_event.stopped_propagation())
-        event->StopPropagation();
-      else
-        event->SetHandled();
-      // Last ui::ET_GESTURE_END should not set the gesture_handler_.
-      if (gesture_event.type() == ui::ET_GESTURE_END) {
-        DCHECK_EQ(1, event->details().touch_points());
-        gesture_handler_ = NULL;
-      }
-      return;
-    }
-
-    // The gesture event wasn't processed. Go up the view hierarchy and
-    // dispatch the gesture event.
-    gesture_handler_ = static_cast<View*>(
-        targeter()->FindNextBestTarget(gesture_handler_, event));
+    // The event was not handled by |target|, so continue processing by
+    // re-targeting the event.
+    target = static_cast<View*>(targeter()->FindNextBestTarget(target, event));
   }
 
-  gesture_handler_ = NULL;
+  // |event| was not handled, so if |gesture_handler_| was not set by the
+  // dispatch of a previous gesture event, then no default gesture handler
+  // should be set prior to the next gesture event being received.
+  // TODO(tdanderson): Move this into a new virtual function
+  //                   EventProcessor::OnEventProcessingFinished(), to be called
+  //                   at the end of EventProcessor::OnEventFromSource().
+  if (!gesture_handler_set_before_dispatch)
+    gesture_handler_ = NULL;
 }
 
 void RootView::UpdateCursor(const ui::MouseEvent& event) {
   if (!(event.flags() & ui::EF_IS_NON_CLIENT)) {
     View* v = GetEventHandlerForPoint(event.location());
     ui::MouseEvent me(event, static_cast<View*>(this), v);
     widget_->SetCursor(v->GetCursor(me));
   }
 }
 
@@ skipping 23 matching lines @@
     }
   }
 }
 
 bool RootView::CanDispatchToTarget(ui::EventTarget* target) {
   return event_dispatch_target_ == target;
 }
 
 ui::EventDispatchDetails RootView::PreDispatchEvent(ui::EventTarget* target,
                                                     ui::Event* event) {
+  View* view = static_cast<View*>(target);
+  if (event->IsGestureEvent()) {
+    // Update |gesture_handler_| to indicate which View is currently handling
+    // gesture events.
+    gesture_handler_ = view;

[sadrul, 2014/08/29 21:43:54]

Instead of setting gesture_handler_ here, can we set it in PostDispatchEvent, if
event.handled()? (you can look into this in the next patch)

[tdanderson, 2014/08/29 21:58:07]

We need to set it in PreDispatchEvent() otherwise there will be no way to detect
the case when a target has been removed from the tree during event bubbling (see
RootViewTargeter::FindNextBestTargetForGestureEvent()).

[sadrul, 2014/09/02 14:36:01]

Should we use |RV::event_dispatch_target_| to determine if the handler has been
removed from the tree instead?

[tdanderson, 2014/09/02 17:26:35]

I like this idea in theory, but for it to work properly I believe we'd need to
set |event_dispatch_target_| to |new_mh| in RootView::SetMouseHandler() so that
|event_dispatch_target_| is reset in cases that call SetMouseHandler(NULL). I'm
hesitant to make  such a change until I start working on mouse events and have a
better understanding of how all the mouse-related members work and what they're
used for. I suspect that SetMouseHandler() will eventually need to be split into
two or more different methods (or at the very least be re-named).

+
+    // Disabled views are permitted to be targets of gesture events, but
+    // gesture events should never actually be dispatched to them. Prevent
+    // dispatch by marking the event as handled.
+    if (!view->enabled())
+      event->SetHandled();
+  }
+
   old_dispatch_target_ = event_dispatch_target_;
-  event_dispatch_target_ = static_cast<View*>(target);
+  event_dispatch_target_ = view;
   return DispatchDetails();
 }
 
 ui::EventDispatchDetails RootView::PostDispatchEvent(ui::EventTarget* target,
                                                      const ui::Event& event) {
+  // The GESTURE_END event corresponding to the removal of the final touch
+  // point marks the end of a gesture sequence, so reset |gesture_handler_|
+  // to NULL if the event will not be re-targeted further.
+  if (event.type() == ui::ET_GESTURE_END &&
+      (!allow_gesture_event_retargeting_ || event.handled())) {

[sadrul, 2014/08/29 21:43:54]

This seems wrong. Shouldn't we always reset gesture_handler_ to NULL on
ET_GESTURE_END?

[tdanderson, 2014/08/29 21:58:07]

We shouldn't reset |gesture_handler_| to NULL until the GESTURE_END event has
finished bubbling up the views hierarchy. Setting it to NULL in the way you
suggest would actually prevent it from bubbling, which is wrong.

[sadrul, 2014/09/02 14:36:01]

Do we need GESTURE_END to bubble up? (i.e. shouldn't we dispatch the GESTURE_END
to a View only if it had already processed some other gesture event (thus
becoming |gesture_handler_|))?

[tdanderson, 2014/09/02 17:26:35]

Yes, this makes sense. I will put up a separate CL to be landed before this one.

[tdanderson, 2014/09/02 20:29:06]

https://codereview.chromium.org/533793002/ has landed, so I adjusted this
accordingly in the next patch set.

+    // In case a drag was in progress, reset all the handlers. Otherwise, just
+    // reset the gesture handler.
+    if (gesture_handler_ == mouse_pressed_handler_)
+      SetMouseHandler(NULL);
+    else
+      gesture_handler_ = NULL;
+  }
+
   DispatchDetails details;
   if (target != event_dispatch_target_)
     details.target_destroyed = true;
 
   event_dispatch_target_ = old_dispatch_target_;
   old_dispatch_target_ = NULL;
 
 #ifndef NDEBUG
   DCHECK(!event_dispatch_target_ || Contains(event_dispatch_target_));
 #endif
 
   return details;
 }
 
 }  // namespace internal
 }  // namespace views