Minor cleanups in GestureEventQueue

In https://crrev.com/c/1332769 we removed the vsync aligned feature
flag. This caused the removal of much code in GestureEventQueue. Namely,
it no longer attempts to coalesce and queue events.

This CL cleans up the nomenclature and commentary in the class to better
reflect its current duties.

Bug: 625689
Change-Id: I295fec7923b8d251853b9522c54641edfcac076a
Reviewed-on: https://chromium-review.googlesource.com/c/1334203Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607998}

content/browser/renderer_host/input/gesture_event_queue.cc

@@ -42,7 +42,7 @@ GestureEventQueue::GestureEventQueue(
 
 GestureEventQueue::~GestureEventQueue() { }
 
-bool GestureEventQueue::DebounceOrQueueEvent(
+bool GestureEventQueue::DebounceOrForwardEvent(
     const GestureEventWithLatencyInfo& gesture_event) {
   // GFS and GFC should have been filtered in FlingControllerFilterEvent.
   DCHECK_NE(gesture_event.event.GetType(), WebInputEvent::kGestureFlingStart);
@@ -50,7 +50,7 @@ bool GestureEventQueue::DebounceOrQueueEvent(
   if (!ShouldForwardForBounceReduction(gesture_event))
     return false;
 
-  QueueAndForwardIfNecessary(gesture_event);
+  ForwardGestureEvent(gesture_event);
   return true;
 }
 
@@ -136,7 +136,6 @@ bool GestureEventQueue::ShouldForwardForBounceReduction(
     case WebInputEvent::kGesturePinchBegin:
     case WebInputEvent::kGesturePinchEnd:
     case WebInputEvent::kGesturePinchUpdate:
-      // TODO(rjkroege): Debounce pinch (http://crbug.com/147647)
       return true;
     default:
       if (scrolling_in_progress_) {
@@ -147,13 +146,13 @@ bool GestureEventQueue::ShouldForwardForBounceReduction(
   }
 }
 
-void GestureEventQueue::QueueAndForwardIfNecessary(
+void GestureEventQueue::ForwardGestureEvent(
     const GestureEventWithLatencyInfo& gesture_event) {
   // GFS and GFC should have been filtered in FlingControllerFilterEvent to
   // get handled by fling controller.
   DCHECK_NE(gesture_event.event.GetType(), WebInputEvent::kGestureFlingStart);
   DCHECK_NE(gesture_event.event.GetType(), WebInputEvent::kGestureFlingCancel);
-  coalesced_gesture_events_.push_back(gesture_event);
+  sent_events_awaiting_ack_.push_back(gesture_event);
   if (gesture_event.event.GetType() == WebInputEvent::kGestureScrollBegin) {
     fling_controller_.RegisterFlingSchedulerObserver();
   } else if (gesture_event.event.GetType() ==
@@ -161,7 +160,6 @@ void GestureEventQueue::QueueAndForwardIfNecessary(
     fling_controller_.UnregisterFlingSchedulerObserver();
   }
   client_->SendGestureEventImmediately(gesture_event);
-  return;
 }
 
 void GestureEventQueue::ProcessGestureAck(InputEventAckSource ack_source,
@@ -170,14 +168,14 @@ void GestureEventQueue::ProcessGestureAck(InputEventAckSource ack_source,
                                           const ui::LatencyInfo& latency) {
   TRACE_EVENT0("input", "GestureEventQueue::ProcessGestureAck");
 
-  if (coalesced_gesture_events_.empty()) {
+  if (sent_events_awaiting_ack_.empty()) {
     DLOG(ERROR) << "Received unexpected ACK for event type " << type;
     return;
   }
 
   // ACKs could come back out of order. We want to cache them to restore the
   // original order.
-  for (auto& outstanding_event : coalesced_gesture_events_) {
+  for (auto& outstanding_event : sent_events_awaiting_ack_) {
     if (outstanding_event.ack_state() != INPUT_EVENT_ACK_STATE_UNKNOWN)
       continue;
     if (outstanding_event.event.GetType() == type) {
@@ -196,12 +194,12 @@ void GestureEventQueue::AckCompletedEvents() {
   if (processing_acks_)
     return;
   base::AutoReset<bool> process_acks(&processing_acks_, true);
-  while (!coalesced_gesture_events_.empty()) {
-    auto iter = coalesced_gesture_events_.begin();
+  while (!sent_events_awaiting_ack_.empty()) {
+    auto iter = sent_events_awaiting_ack_.begin();
     if (iter->ack_state() == INPUT_EVENT_ACK_STATE_UNKNOWN)
       break;
     GestureEventWithLatencyInfoAndAckState event = *iter;
-    coalesced_gesture_events_.erase(iter);
+    sent_events_awaiting_ack_.erase(iter);
     AckGestureEventToClient(event, event.ack_source(), event.ack_state());
   }
 }
@@ -210,9 +208,6 @@ void GestureEventQueue::AckGestureEventToClient(
     const GestureEventWithLatencyInfo& event_with_latency,
     InputEventAckSource ack_source,
     InputEventAckState ack_result) {
-  // Ack'ing an event may enqueue additional gesture events.  By ack'ing the
-  // event before the forwarding of queued events below, such additional events
-  // can be coalesced with existing queued events prior to dispatch.
   client_->OnGestureEventAck(event_with_latency, ack_source, ack_result);
 }
 
@@ -221,11 +216,6 @@ TouchpadTapSuppressionController*
   return fling_controller_.GetTouchpadTapSuppressionController();
 }
 
-void GestureEventQueue::ForwardGestureEvent(
-    const GestureEventWithLatencyInfo& gesture_event) {
-  QueueAndForwardIfNecessary(gesture_event);
-}
-
 void GestureEventQueue::SendScrollEndingEventsNow() {
   scrolling_in_progress_ = false;
   if (debouncing_deferral_queue_.empty())
@@ -235,7 +225,7 @@ void GestureEventQueue::SendScrollEndingEventsNow() {
   for (GestureQueue::const_iterator it = debouncing_deferral_queue.begin();
        it != debouncing_deferral_queue.end(); it++) {
     if (!fling_controller_.FilterGestureEvent(*it)) {
-      QueueAndForwardIfNecessary(*it);
+      ForwardGestureEvent(*it);
     }
   }
 }

content/browser/renderer_host/input/gesture_event_queue.h

@@ -37,26 +37,28 @@ class CONTENT_EXPORT GestureEventQueueClient {
                                  InputEventAckState ack_result) = 0;
 };
 
-// Maintains WebGestureEvents in a queue before forwarding them to the renderer
-// to apply a sequence of filters on them:
+// Despite its name, this class isn't so much one queue as it is a collection
+// of queues and filters. This class applies logic to determine if an event
+// should be queued, filtered altogether, or sent immediately; it tracks sent
+// events and ACKs them to the clilent in the order they were dispatched. This
+// class applies a series of filters and queues for various scenarios:
 // 1. The sequence is filtered for bounces. A bounce is when the finger lifts
-//    from the screen briefly during an in-progress scroll. Ifco this happens,
+//    from the screen briefly during an in-progress scroll. If this happens,
 //    non-GestureScrollUpdate events are queued until the de-bounce interval
 //    passes or another GestureScrollUpdate event occurs.
 // 2. Unnecessary GestureFlingCancel events are filtered by fling controller.
 //    These are GestureFlingCancels that have no corresponding GestureFlingStart
-//    in the queue.
+//    in the queue. GestureFlingStarts are also filtered and translated to
+//    scroll gestures by the fling controller.
 // 3. Taps immediately after a GestureFlingCancel (caused by the same tap) are
 //    filtered by fling controller.
-// 4. Whenever possible, events in the queue are coalesced to have as few events
-//    as possible and therefore maximize the chance that the event stream can be
-//    handled entirely by the compositor thread.
-// Events in the queue are forwarded to the renderer one by one; i.e., each
-// event is sent after receiving the ACK for previous one. The only exception is
-// that if a GestureScrollUpdate is followed by a GesturePinchUpdate, they are
-// sent together.
-// TODO(rjkroege): Possibly refactor into a filter chain:
-// http://crbug.com/148443.
+// 4. Gesture events are queued while we're waiting to determine the allowed
+//    touch actions.
+// Sent events are kept in a queue until a response from the renderer is
+// received for that event. The client is notified of ACKs in the order the
+// events were sent, not ACK'd. This means an ACK'd event that was sent after
+// an event still awaiting an ACK won't notify the client until the earlier
+// event is ACK'd.
 class CONTENT_EXPORT GestureEventQueue {
  public:
   using GestureQueue = base::circular_deque<GestureEventWithLatencyInfo>;
@@ -79,24 +81,23 @@ class CONTENT_EXPORT GestureEventQueue {
   ~GestureEventQueue();
 
   // Uses fling controller to filter the gesture event. Returns true if the
-  // event wasn't queued and was filtered.
+  // event was filtered by the fling controller and shouldn't be further
+  // forwarded.
   bool FlingControllerFilterEvent(const GestureEventWithLatencyInfo&);
 
-  // Check for debouncing, or add the gesture event to the queue. Returns false
-  // if the event wasn't queued.
-  bool DebounceOrQueueEvent(const GestureEventWithLatencyInfo&);
+  // Filter the event for debouncing or forward it to the renderer. Returns
+  // true if the event was forwarded, false if was filtered for debouncing.
+  bool DebounceOrForwardEvent(const GestureEventWithLatencyInfo&);
 
   // Adds a gesture to the queue of events that needs to be deferred until the
   // touch action is known.
   void QueueDeferredEvents(const GestureEventWithLatencyInfo&);
 
-  // Returns events in the |deferred_gesture_queue_| and empty
-  // the queue.
+  // Returns events in the |deferred_gesture_queue_| and empty the queue.
   GestureQueue TakeDeferredEvents();
 
   // Indicates that the caller has received an acknowledgement from the renderer
-  // with state |ack_result| and event |type|. May send events if the queue is
-  // not empty.
+  // with state |ack_result| and event |type|.
   void ProcessGestureAck(InputEventAckSource ack_source,
                          InputEventAckState ack_result,
                          blink::WebInputEvent::Type type,
@@ -105,10 +106,12 @@ class CONTENT_EXPORT GestureEventQueue {
   // Returns the |TouchpadTapSuppressionController| instance.
   TouchpadTapSuppressionController* GetTouchpadTapSuppressionController();
 
+  // Sends the gesture event to the renderer. Stores the sent event for when
+  // the renderer replies with an ACK.
   void ForwardGestureEvent(const GestureEventWithLatencyInfo& gesture_event);
 
   bool empty() const {
-    return coalesced_gesture_events_.empty() &&
+    return sent_events_awaiting_ack_.empty() &&
            debouncing_deferral_queue_.empty();
   }
 
@@ -151,12 +154,6 @@ class CONTENT_EXPORT GestureEventQueue {
   bool ShouldForwardForBounceReduction(
       const GestureEventWithLatencyInfo& gesture_event);
 
-  // Puts the events in a queue to forward them one by one; i.e., forward them
-  // whenever ACK for previous event is received. This queue also tries to
-  // coalesce events as much as possible.
-  void QueueAndForwardIfNecessary(
-      const GestureEventWithLatencyInfo& gesture_event);
-
   // ACK completed events in order until we have reached an incomplete event.
   // Will preserve the FIFO order as events originally arrived.
   void AckCompletedEvents();
@@ -178,8 +175,12 @@ class CONTENT_EXPORT GestureEventQueue {
       base::circular_deque<GestureEventWithLatencyInfoAndAckState>;
 
   // Stores outstanding events that have been sent to the renderer but not yet
-  // been ACKed.
-  GestureQueueWithAckState coalesced_gesture_events_;
+  // been ACK'd. These are kept in the order they were sent in so that they can
+  // be ACK'd back in order. Note, the renderer can reply to these out-of-order.
+  // This class makes a note of the ACK state but doesn't actually let the
+  // client know about the ACK until all events earlier in the queue have been
+  // ACK'd so that the client sees the ACKs in order.
+  GestureQueueWithAckState sent_events_awaiting_ack_;
 
   // Timer to release a previously deferred gesture event.
   base::OneShotTimer debounce_deferring_timer_;

content/browser/renderer_host/input/gesture_event_queue_unittest.cc

@@ -119,7 +119,7 @@ class GestureEventQueueTest : public testing::Test,
   void SimulateGestureEvent(const WebGestureEvent& gesture) {
     GestureEventWithLatencyInfo gesture_event(gesture);
     if (!queue()->FlingControllerFilterEvent(gesture_event)) {
-      queue()->DebounceOrQueueEvent(gesture_event);
+      queue()->DebounceOrForwardEvent(gesture_event);
     }
   }
 
@@ -191,16 +191,17 @@ class GestureEventQueueTest : public testing::Test,
   }
 
   unsigned GestureEventQueueSize() {
-    return queue()->coalesced_gesture_events_.size();
+    return queue()->sent_events_awaiting_ack_.size();
   }
 
   WebGestureEvent GestureEventSecondFromLastQueueEvent() {
-    return queue()->coalesced_gesture_events_.at(
-        GestureEventQueueSize() - 2).event;
+    return queue()
+        ->sent_events_awaiting_ack_.at(GestureEventQueueSize() - 2)
+        .event;
   }
 
   WebGestureEvent GestureEventLastQueueEvent() {
-    return queue()->coalesced_gesture_events_.back().event;
+    return queue()->sent_events_awaiting_ack_.back().event;
   }
 
   unsigned GestureEventDebouncingQueueSize() {
@@ -208,7 +209,7 @@ class GestureEventQueueTest : public testing::Test,
   }
 
   WebGestureEvent GestureEventQueueEventAt(int i) {
-    return queue()->coalesced_gesture_events_.at(i).event;
+    return queue()->sent_events_awaiting_ack_.at(i).event;
   }
 
   bool ScrollingInProgress() {

content/browser/renderer_host/input/input_router_impl.cc

@@ -194,7 +194,7 @@ void InputRouterImpl::SendGestureEventWithoutQueueing(
     return;
   }
 
-  if (!gesture_event_queue_.DebounceOrQueueEvent(gesture_event)) {
+  if (!gesture_event_queue_.DebounceOrForwardEvent(gesture_event)) {
     disposition_handler_->OnGestureEventAck(gesture_event,
                                             InputEventAckSource::BROWSER,
                                             INPUT_EVENT_ACK_STATE_CONSUMED);