Coalesce async touch move events until the ack back from render

content/browser/renderer_host/input/input_router_impl.cc

Index: content/browser/renderer_host/input/input_router_impl.cc
diff --git a/content/browser/renderer_host/input/input_router_impl.cc b/content/browser/renderer_host/input/input_router_impl.cc
index df02bb43ac6becfd7378a6aa54290f0d2112dd51..a51150c0a742a0acf03f4213bf4138a34b25345a 100644
--- a/content/browser/renderer_host/input/input_router_impl.cc
+++ b/content/browser/renderer_host/input/input_router_impl.cc
@@ -356,19 +356,22 @@ void InputRouterImpl::OfferToHandlers(const WebInputEvent& input_event,
   OfferToRenderer(input_event, latency_info, is_keyboard_shortcut);
 
   // Touch events should always indicate in the event whether they are
-  // cancelable (respect ACK disposition) or not.
-  bool ignores_ack = WebInputEventTraits::IgnoresAckDisposition(input_event);
-  if (WebInputEvent::isTouchEventType(input_event.type)) {
+  // cancelable (respect ACK disposition) or not except touchmove.
+  bool needs_synthetic_ack =
+      WebInputEventTraits::WillReceiveAckFromRenderer(input_event);

[jdduke (slow), 2015/05/07 21:11:15]

I think you're missing a "!" here.

[lanwei, 2015/05/08 19:31:25]

Not really, because we change both the function name and variable name, they
mean the same thing right now, and I revert the condition below.

+
+  if (WebInputEvent::isTouchEventType(input_event.type) &&
+      input_event.type != WebInputEvent::TouchMove) {
     const WebTouchEvent& touch = static_cast<const WebTouchEvent&>(input_event);
-    DCHECK_NE(ignores_ack, !!touch.cancelable);
+    DCHECK_EQ(needs_synthetic_ack, touch.cancelable);
   }
 
-  // If we don't care about the ack disposition, send the ack immediately.
-  if (ignores_ack) {
-    ProcessInputEventAck(input_event.type,
-                         INPUT_EVENT_ACK_STATE_IGNORED,
-                         latency_info,
-                         IGNORING_DISPOSITION);
+  // If we don't need synthetic ack from render, send the ack immediately.
+  if (!needs_synthetic_ack) {
+    InputEventAck ack(input_event.type, INPUT_EVENT_ACK_STATE_IGNORED,

[jdduke (slow), 2015/05/07 21:11:15]

Looking at this now, I can see why you wondered about why we need the new
struct. I guess it doesn't actually save us any code.

I had originally anticipated that we would send this ack throughout the pipeline
(i.e., use it instead of InputEventAckState when acking to the router's client,
and the touch queue, etc...), but maybe that refactoring should be done
separately.

+                      latency_info,
+                      WebInputEventTraits::GetUniqueTouchEventId(input_event));
+    ProcessInputEventAck(ack, IGNORING_DISPOSITION);
   }
 }
 
@@ -380,13 +383,17 @@ bool InputRouterImpl::OfferToClient(const WebInputEvent& input_event,
       client_->FilterInputEvent(input_event, latency_info);
   switch (filter_ack) {
     case INPUT_EVENT_ACK_STATE_CONSUMED:
-    case INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS:
+    case INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS: {
       // Send the ACK and early exit.
       next_mouse_move_.reset();
-      ProcessInputEventAck(input_event.type, filter_ack, latency_info, CLIENT);
+      InputEventAck ack(
+          input_event.type, filter_ack, latency_info,
+          WebInputEventTraits::GetUniqueTouchEventId(input_event));
+      ProcessInputEventAck(ack, CLIENT);
       // WARNING: |this| may be deleted at this point.
       consumed = true;
       break;
+    }
     case INPUT_EVENT_ACK_STATE_UNKNOWN:
       // Simply drop the event.
       consumed = true;
@@ -406,7 +413,7 @@ bool InputRouterImpl::OfferToRenderer(const WebInputEvent& input_event,
     // Ack messages for ignored ack event types should never be sent by the
     // renderer. Consequently, such event types should not affect event time
     // or in-flight event count metrics.
-    if (!WebInputEventTraits::IgnoresAckDisposition(input_event)) {
+    if (WebInputEventTraits::WillReceiveAckFromRenderer(input_event)) {
       input_event_start_time_ = TimeTicks::Now();
       client_->IncrementInFlightEventCount();
     }
@@ -415,10 +422,8 @@ bool InputRouterImpl::OfferToRenderer(const WebInputEvent& input_event,
   return false;
 }
 
-void InputRouterImpl::OnInputEventAck(
-    const InputHostMsg_HandleInputEvent_ACK_Params& ack) {
+void InputRouterImpl::OnInputEventAck(const InputEventAck& ack) {
   client_->DecrementInFlightEventCount();
-
   // Log the time delta for processing an input event.
   TimeDelta delta = TimeTicks::Now() - input_event_start_time_;
   UMA_HISTOGRAM_TIMES("MPArch.IIR_InputEventDelta", delta);
@@ -429,21 +434,7 @@ void InputRouterImpl::OnInputEventAck(
     OnDidOverscroll(*ack.overscroll);
   }
 
-  ProcessInputEventAck(ack.type, ack.state, ack.latency, RENDERER);
-  // WARNING: |this| may be deleted at this point.
-
-  // This is used only for testing, and the other end does not use the
-  // source object.  On linux, specifying
-  // Source<RenderWidgetHost> results in a very strange
-  // runtime error in the epilogue of the enclosing
-  // (ProcessInputEventAck) method, but not on other platforms; using
-  // 'void' instead is just as safe (since NotificationSource
-  // is not actually typesafe) and avoids this error.
-  int type = static_cast<int>(ack.type);
-  NotificationService::current()->Notify(
-      NOTIFICATION_RENDER_WIDGET_HOST_DID_RECEIVE_INPUT_EVENT_ACK,
-      Source<void>(this),
-      Details<int>(&type));
+  ProcessInputEventAck(ack, RENDERER);
 }
 
 void InputRouterImpl::OnDidOverscroll(const DidOverscrollParams& params) {
@@ -502,11 +493,10 @@ void InputRouterImpl::OnDidStopFlinging() {
   client_->DidStopFlinging();
 }
 
-void InputRouterImpl::ProcessInputEventAck(
-    WebInputEvent::Type event_type,

[jdduke (slow), 2015/05/07 21:11:15]

For now let's keep this signature (adding a touch id argument) so we avoid
creating the temporary ack structures that I guess aren't really necessary as
we're not propagating them to the client. Maybe at some point we can switch, but
for now let's avoid the overhead.

[lanwei, 2015/05/08 19:31:25]

Done.

-    InputEventAckState ack_result,
-    const ui::LatencyInfo& latency_info,
-    AckSource ack_source) {
+void InputRouterImpl::ProcessInputEventAck(const InputEventAck& ack,
+                                           AckSource ack_source) {
+  blink::WebInputEvent::Type event_type = ack.type;
+  InputEventAckState ack_result = ack.state;
   TRACE_EVENT2("input", "InputRouterImpl::ProcessInputEventAck",
                "type", WebInputEventTraits::GetName(event_type),
                "ack", GetEventAckName(ack_result));
@@ -526,11 +516,11 @@ void InputRouterImpl::ProcessInputEventAck(
   if (WebInputEvent::isMouseEventType(event_type)) {
     ProcessMouseAck(event_type, ack_result);
   } else if (event_type == WebInputEvent::MouseWheel) {
-    ProcessWheelAck(ack_result, latency_info);
+    ProcessWheelAck(ack_result, ack.latency);
   } else if (WebInputEvent::isTouchEventType(event_type)) {
-    ProcessTouchAck(ack_result, latency_info);
+    ProcessTouchAck(ack_result, ack.latency, ack.unique_touch_event_id);
   } else if (WebInputEvent::isGestureEventType(event_type)) {
-    ProcessGestureAck(event_type, ack_result, latency_info);
+    ProcessGestureAck(event_type, ack_result, ack.latency);
   } else if (event_type != WebInputEvent::Undefined) {
     ack_handler_->OnUnexpectedEventAck(InputAckHandler::BAD_ACK_MESSAGE);
   }
@@ -607,11 +597,12 @@ void InputRouterImpl::ProcessGestureAck(WebInputEvent::Type type,
   gesture_event_queue_.ProcessGestureAck(ack_result, type, latency);
 }
 
-void InputRouterImpl::ProcessTouchAck(
-    InputEventAckState ack_result,
-    const ui::LatencyInfo& latency) {
+void InputRouterImpl::ProcessTouchAck(InputEventAckState ack_result,
+                                      const ui::LatencyInfo& latency,
+                                      const uint32 unique_touch_event_id) {

[jdduke (slow), 2015/05/07 21:11:15]

Hmm, passing by const value is rare, let's just pass a regular uint32.

[lanwei, 2015/05/08 19:31:25]

Done.

   // |touch_event_queue_| will forward to OnTouchEventAck when appropriate.
-  touch_event_queue_.ProcessTouchAck(ack_result, latency);
+  touch_event_queue_.ProcessTouchAck(ack_result, latency,
+                                     unique_touch_event_id);
 }
 
 void InputRouterImpl::UpdateTouchAckTimeoutEnabled() {