Avoid sending touchmove events where all pointers are stationary

Avoid sending touchmove events where all pointers are stationary

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. Now, TouchEventQueue filters these events and
avoids sending the events to the renderer.

BUG=452032
Committed: https://crrev.com/f50adc73e0ccf91c48ecbfad127e45ab4d5129cb
Cr-Commit-Position: refs/heads/master@{#316609}

[USE s.singapati at gmail.com, 2015-02-11 15:15:41]

s.singapati@samsung.com changed reviewers:
+ jdduke@chromium.org

[USE s.singapati at gmail.com, 2015-02-11 15:15:42]

Hi, got some time to work on it. some comments on the patch:

Currently |FilterBeforeForwarding| intends to filter (/filtering) events with
ACK_WITH_NO_CONSUMER_EXISTS when all points are StateStationary.
This patch does actual checking of stationary points in TouchMoves, and retains
the same behavior currently by sending ACK_WITH_NO_CONSUMER_EXISTS.
Otherwise, it needs to be acked with ACK_WITH_NOT_CONSUMED, as currently being
done from |EventHandler::handleTouchEvent|.
(discussion in https://codereview.chromium.org/788923002)

Refactoring both filtering and updating of pointer states in the same place
(|FilterBeforeForwarding|) may be done but seems to take more changes in api
(changing const& vs pointers) because of public |QueueEvent|. 
Some of unit tests have been updated to really move the touch points.

Or does it have totally different implementation plan? Any comments? Thank you.

[jdduke (slow), 2015-02-11 17:17:13]

Looks reasonable.

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:50: const
WebTouchPoint& point_2) {
Nit: Will this now fit on the same line?

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

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:739: if
(HasPointChanged(last_sent_touchevent_->touches[j], point))
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]

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:50: const
WebTouchPoint& point_2) {
On 2015/02/11 17:17:12, jdduke wrote:
> Nit: Will this now fit on the same line?
line exceeds 80.

> Also, maybe we should add a DCHECK_EQ(point_1.id, point_2.id).
yes, DCHECK_EQ is fine.

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:739: if
(HasPointChanged(last_sent_touchevent_->touches[j], point))
On 2015/02/11 17:17:12, jdduke wrote:
> Can we invert this condition?
> 
> if (event.type == WebInputEvent::TouchMove) {
>   if (!HasPointChanged(...))
>     return ACK_WITH_NOT_CONSUMED
> }
> return FORWARD_TO_RENDERER;
> 
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.

> Simple to follow, and no need for the break statement.
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]

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:739: if
(HasPointChanged(last_sent_touchevent_->touches[j], point))
On 2015/02/12 11:15:02, s.singapati wrote:
> On 2015/02/11 17:17:12, jdduke wrote:
> > Can we invert this condition?
> > 
> > if (event.type == WebInputEvent::TouchMove) {
> >   if (!HasPointChanged(...))
> >     return ACK_WITH_NOT_CONSUMED
> > }
> > return FORWARD_TO_RENDERER;
> > 
> 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.
> 
> > Simple to follow, and no need for the break statement.
> 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;

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:48]

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:50: const
WebTouchPoint& point_2) {
On 2015/02/12 11:15:02, s.singapati wrote:
> On 2015/02/11 17:17:12, jdduke wrote:
> > Nit: Will this now fit on the same line?
> line exceeds 80.
> 
> > Also, maybe we should add a DCHECK_EQ(point_1.id, point_2.id).
> yes, DCHECK_EQ is fine.

Done.

https://codereview.chromium.org/916103002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_event_queue.cc:739: if
(HasPointChanged(last_sent_touchevent_->touches[j], point))
On 2015/02/12 16:55:56, jdduke wrote:
> On 2015/02/12 11:15:02, s.singapati wrote:
> > On 2015/02/11 17:17:12, jdduke wrote:
> > > Can we invert this condition?
> > > 
> > > if (event.type == WebInputEvent::TouchMove) {
> > >   if (!HasPointChanged(...))
> > >     return ACK_WITH_NOT_CONSUMED
> > > }
> > > return FORWARD_TO_RENDERER;
> > > 
> > 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.
> > 
> > > Simple to follow, and no need for the break statement.
> > 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;
> 
> 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.

Done.

[jdduke (slow), 2015-02-13 16:37:37]

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:745: // This is a
TouchMove event and not forwarded yet.
Nit: Maybe reword this like:

// This is a TouchMove event for which we have yet to find a non-stationary
pointer. Continue checking the next pointers in the |event|.

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:748: }
It's a shame we can't "clean up" the pointer state here while we're at it, but
that would involve making the argument a pointer and maybe it's not worth it.

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:753: if (event.type ==
WebInputEvent::TouchMove)
This isn't always correct. Let's just go ahead and stick with NO_CONSUMER_EXISTS
for now.

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1505:
EXPECT_EQ(INPUT_EVENT_ACK_STATE_NOT_CONSUMED, acked_event_state());
This doesn't look right, see my other comment. Let's just stick with
NO_CONSUMER_EXISTS for now.

[USE s.singapati at gmail.com, 2015-02-16 10:52:01]

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:745: // This is a
TouchMove event and not forwarded yet.
On 2015/02/13 16:37:37, jdduke wrote:
> Nit: Maybe reword this like:
> 
> // This is a TouchMove event for which we have yet to find a non-stationary
> pointer. Continue checking the next pointers in the |event|.
> 

Done.

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:748: }
On 2015/02/13 16:37:37, jdduke wrote:
> It's a shame we can't "clean up" the pointer state here while we're at it, but
> that would involve making the argument a pointer and maybe it's not worth it.

Acknowledged. Lets see if this can be done may be after this patch.

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:753: if (event.type ==
WebInputEvent::TouchMove)
On 2015/02/13 16:37:37, jdduke wrote:
> This isn't always correct. Let's just go ahead and stick with
NO_CONSUMER_EXISTS
> for now.

Done.

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/916103002/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:1505:
EXPECT_EQ(INPUT_EVENT_ACK_STATE_NOT_CONSUMED, acked_event_state());
On 2015/02/13 16:37:37, jdduke wrote:
> This doesn't look right, see my other comment. Let's just stick with
> NO_CONSUMER_EXISTS for now.

Done.

[USE s.singapati at gmail.com, 2015-02-17 15:57:06]

jdduke: Any other comments on latest patch set?

[jdduke (slow), 2015-02-17 16:28:26]

On 2015/02/17 15:57:06, s.singapati wrote:
> jdduke: Any other comments on latest patch set?

lgtm (sorry for the delay, yesterday was a holiday).

[USE s.singapati at gmail.com, 2015-02-17 17:22:47]

On 2015/02/17 16:28:26, jdduke wrote:
> On 2015/02/17 15:57:06, s.singapati wrote:
> > jdduke: Any other comments on latest patch set?
> 
> lgtm (sorry for the delay, yesterday was a holiday).

Thank you for reviews.

[USE s.singapati at gmail.com, 2015-02-17 17:23:07]

The CQ bit was checked by s.singapati@samsung.com

[commit-bot: I haz the power, 2015-02-17 17:24:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/916103002/40001

[commit-bot: I haz the power, 2015-02-17 18:12:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-17 18:13:42]

Patchset 3 (id:??) landed as
https://crrev.com/f50adc73e0ccf91c48ecbfad127e45ab4d5129cb
Cr-Commit-Position: refs/heads/master@{#316609}