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_ =
+  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;
+  // If |allow_gesture_event_retargeting_| is true, then |gesture_handler_| was
+  // not set by the dispatch of a previous gesture event. Since |event| was

[tdanderson, 2014/08/28 21:15:36]

Another reason why I didn't want to name this member
|allow_gesture_event_retargeting_| in the first place was to avoid awkward
explanations such as the first sentence of this comment. Do you think this
documentation would make the logic understandable to someone reading through
this code for the first time?

[sadrul, 2014/08/29 19:35:53]

I am having a hard time understanding this myself.

Can you add:
  bool gesture_handler_set_before_dispatch = !!gesture_handler_;

at the beginning of this function, and use that here instead?

  if (!gesture_handler_set_before_dispatch)
    gesture_handler_ = NULL;

Or maybe use |gesture_handler_set_before_dispatch_| instead of
|allow_gesture_event_regargeting_|

[tdanderson, 2014/08/29 21:19:51]

Implemented your first suggestion for the time being. Once
DispatchGestureEvent() is deleted then I will take a look over all documentation
again to see if the variable name is clear.

+  // not handled, 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 (allow_gesture_event_retargeting_)
+    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;
+
+    // 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())) {

[tdanderson, 2014/08/28 21:15:36]

Won't the approach of tracking |gesture_handler_| and
|allow_gesture_event_retargeting_| be problematic in cases where we want to have
nested dispatch of gesture events? The processing of the inner event could
change the values of these members and they wouldn't be changed back when the
outer event's processing is resumed.

+    // 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