Avoid sending touchmove events where all pointers are stationary

content/browser/renderer_host/input/touch_event_queue.cc

 // Copyright 2013 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 "content/browser/renderer_host/input/touch_event_queue.h"
 
 #include "base/auto_reset.h"
 #include "base/stl_util.h"
 #include "base/trace_event/trace_event.h"
 #include "content/browser/renderer_host/input/timeout_monitor.h"
@@ skipping 28 matching lines @@
   return event;
 }
 
 bool ShouldTouchTriggerTimeout(const WebTouchEvent& event) {
   return (event.type == WebInputEvent::TouchStart ||
           event.type == WebInputEvent::TouchMove) &&
          !WebInputEventTraits::IgnoresAckDisposition(event);
 }
 
 // Compare all properties of touch points to determine the state.
-bool HasPointChanged(const WebTouchPoint& last_point,
-    const WebTouchPoint& current_point) {
-  if (last_point.screenPosition != current_point.screenPosition ||
-      last_point.position != current_point.position ||
-      last_point.radiusX != current_point.radiusX ||
-      last_point.radiusY != current_point.radiusY ||
-      last_point.rotationAngle != current_point.rotationAngle ||
-      last_point.force != current_point.force) {
+bool HasPointChanged(const WebTouchPoint& point_1,
+    const WebTouchPoint& point_2) {

[jdduke (slow), 2015/02/11 17:17:12]

Nit: Will this now fit on the same line?

Also, maybe we should add a DCHECK_EQ(point_1.id, point_2.id).

[USE s.singapati at gmail.com, 2015/02/12 11:15:02]

line exceeds 80.
yes, DCHECK_EQ is fine.

[USE s.singapati at gmail.com, 2015/02/13 12:16:48]

Done.

+  if (point_1.screenPosition != point_2.screenPosition ||
+      point_1.position != point_2.position ||
+      point_1.radiusX != point_2.radiusX ||
+      point_1.radiusY != point_2.radiusY ||
+      point_1.rotationAngle != point_2.rotationAngle ||
+      point_1.force != point_2.force) {
     return true;
   }
   return false;
 }
 
 }  // namespace
 
 
 // Cancels a touch sequence if a touchstart or touchmove ack response is
 // sufficiently delayed.
@@ skipping 655 matching lines @@
       const WebTouchPoint& point = event.touches[i];
       if (point.state == WebTouchPoint::StateStationary)
         continue;
 
       // |last_sent_touchevent_| will be non-null as long as there is an
       // active touch sequence being forwarded to the renderer.
       if (!last_sent_touchevent_)
         continue;
 
       for (size_t j = 0; j < last_sent_touchevent_->touchesLength; ++j) {
-        if (point.id == last_sent_touchevent_->touches[j].id)
+        if (point.id != last_sent_touchevent_->touches[j].id)
+          continue;
+
+        // It is possible that all the pointers in a given TouchMove may have
+        // state as StateMoved, even though none of the pointers have not
+        // changed in real. Avoid sending these events to the renderer.
+        if (event.type == WebInputEvent::TouchMove) {
+          if (HasPointChanged(last_sent_touchevent_->touches[j], point))

[jdduke (slow), 2015/02/11 17:17:12]

Can we invert this condition?

if (event.type == WebInputEvent::TouchMove) {
  if (!HasPointChanged(...))
    return ACK_WITH_NOT_CONSUMED
}
return FORWARD_TO_RENDERER;

Simple to follow, and no need for the break statement.

[USE s.singapati at gmail.com, 2015/02/12 11:15:02]

Hmm..I think not. ACK_WITH_NOT_CONSUMED should not be sent YET here, because
next pointers in the TouchMove may have been changed in real.
Probably need to update comment for break; statement like
"Pointer with same id found. If this is a TouchMove, then event may not have
been forwarded yet. So, continue handling next pointers in the |event|."

And if the requirement is really to send ACK_WITH_NOT_CONSUMED, then this should
be done at the end of method.

if (event.type == WebInputEvent::TouchMove)
 return ACK_WITH_NOT_CONSUMED;

return ACK_WITH_NO_CONSUMER_EXISTS;

[jdduke (slow), 2015/02/12 16:55:56]

Hmm, yeah I had that completely wrong. It's still a little hard to follow,
maybe:

if (event.type != WebInputEvent::TouchMove)
  return FORWARD_TO_RENDERER;

if (HasPointChanged(...))
  return FORWARD_TO_RENDERER;

break;

with similar comments to what you had.

[USE s.singapati at gmail.com, 2015/02/13 12:16:47]

Done.

+            return FORWARD_TO_RENDERER;
+        } else {
           return FORWARD_TO_RENDERER;
+        }
+        // Pointer with same id found and handled.
+        break;
       }
+
     }
   }
 
   return ACK_WITH_NO_CONSUMER_EXISTS;
 }
 
 void TouchEventQueue::UpdateTouchConsumerStates(const WebTouchEvent& event,
                                                 InputEventAckState ack_result) {
   if (event.type == WebInputEvent::TouchStart) {
     if (ack_result == INPUT_EVENT_ACK_STATE_CONSUMED)
       send_touch_events_async_ = false;
     has_handler_for_current_sequence_ |=
         ack_result != INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS;
   } else if (WebTouchEventTraits::IsTouchSequenceEnd(event)) {
     has_handler_for_current_sequence_ = false;
   }
 }
 
 }  // namespace content