Ensure acks are sent for all blocking events.

Ensure acks are sent for all blocking events.

It was possible for two touch moves that had acks to get coalesced together
and the main thread would only process one and deliver the ack for it. This
caused a situation where the hung renderer timer was getting fired because
there was an outstanding ack in the count. And this timer forced the
omnibox to not go away.

A large portion of this change (plumbing the ack_state will go away when
I land my multi-thread main_thread_event_queue design); but since this
needs to be merged back to M52 this is safest.

BUG=616991
Committed: https://crrev.com/b08721e61d24d65dce50e40731e08231deb95932
Cr-Commit-Position: refs/heads/master@{#402703}

[dtapuska, 2016-06-27 14:11:53]

dtapuska@chromium.org changed reviewers:
+ aelias@chromium.org, tdresser@chromium.org

[dtapuska, 2016-06-27 14:11:53]

aelias@chromium.org: Please review changes in 

tdresser@chromium.org: Please review changes in

[aelias_OOO_until_Jul13, 2016-06-27 21:45:34]

Is there something that can be reverted instead for M52, if this patch is mostly
instantly obsolete code?  (It's fine if your judgement is that it's too
difficult to revert, I'd just like to make sure it was considered.)

[dtapuska, 2016-06-27 23:15:54]

On 2016/06/27 21:45:34, aelias wrote:
> Is there something that can be reverted instead for M52, if this patch is
mostly
> instantly obsolete code?  (It's fine if your judgement is that it's too
> difficult to revert, I'd just like to make sure it was considered.)

Unfortunately not. If we were to revert something it would have to be the
underlying passive event listeners support. It is an obscure scenario that gets
triggered. A passive event in the queue followed by two blocking events.

[aelias_OOO_until_Jul13, 2016-06-27 23:42:14]

OK seems fine then, no strong opinion, I'll let tdresser@ review it.

[tdresser, 2016-06-28 14:04:49]

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_event_filter.h:88: uint32_t touch_event_id)
override;
Add a comment stating what the value of touch_event_id is if this is for
touchpad.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:106: for (const auto id :
in_flight_wheel_event_->eventsToAck)
eventsToAck is just a list of zeroes, which is pretty confusing?

Maybe just loop on the length, to make it clear that you're ignoring the id?

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:20: class EventWithDispatchType
: public BaseClass {
This name is becoming less and less accurate over time - probably fine for now
though.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:28: std::deque<uint32_t>
eventsToAck;
touchEventIdsToAck?

For wheel, we just need a count of the number of acks to send. Using a list of
zeros for a count is a bit weird - we should either split it up and have a
separate count for touchpad, or at least add a comment stating that for
touchpad, this is just an deque of zeros.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:36: // If we are blocking; and
are coalescing touch; make sure to keep
Nit: I think these ; are being used incorrectly - maybe just use ,'s?

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:40:
WebInputEventTraits::GetUniqueTouchEventId(other.event));
We're going to end up pushing a bunch of zero's in the touchpad case, aren't we?

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:132: // TODO(dtapuska): These
can be removed when the queues are dequeued
Can we link to a bug here?

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue_unittest.cc:151:
TEST_F(MainThreadEventQueueTest, BlockingTouch) {
Should we also have a touchpad test of this sort?

[dtapuska, 2016-06-28 17:30:21]

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/inpu...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/inpu...
content/renderer/input/input_event_filter.h:88: uint32_t touch_event_id)
override;
On 2016/06/28 14:04:49, tdresser wrote:
> Add a comment stating what the value of touch_event_id is if this is for
> touchpad.

Done.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:106: for (const auto id :
in_flight_wheel_event_->eventsToAck)
On 2016/06/28 14:04:49, tdresser wrote:
> eventsToAck is just a list of zeroes, which is pretty confusing?
> 
> Maybe just loop on the length, to make it clear that you're ignoring the id?

Changed to be a dcheck.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:20: class EventWithDispatchType
: public BaseClass {
On 2016/06/28 14:04:49, tdresser wrote:
> This name is becoming less and less accurate over time - probably fine for now
> though.

Acknowledged.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:28: std::deque<uint32_t>
eventsToAck;
On 2016/06/28 14:04:49, tdresser wrote:
> touchEventIdsToAck?
> 
> For wheel, we just need a count of the number of acks to send. Using a list of
> zeros for a count is a bit weird - we should either split it up and have a
> separate count for touchpad, or at least add a comment stating that for
> touchpad, this is just an deque of zeros.

I've changed it to be a dcheck for wheel events.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:36: // If we are blocking; and
are coalescing touch; make sure to keep
On 2016/06/28 14:04:49, tdresser wrote:
> Nit: I think these ; are being used incorrectly - maybe just use ,'s?

Done.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:40:
WebInputEventTraits::GetUniqueTouchEventId(other.event));
On 2016/06/28 14:04:49, tdresser wrote:
> We're going to end up pushing a bunch of zero's in the touchpad case, aren't
we?
> 

It shouldn't really happen.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:132: // TODO(dtapuska): These
can be removed when the queues are dequeued
On 2016/06/28 14:04:49, tdresser wrote:
> Can we link to a bug here?

Done.

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2094323002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue_unittest.cc:151:
TEST_F(MainThreadEventQueueTest, BlockingTouch) {
On 2016/06/28 14:04:49, tdresser wrote:
> Should we also have a touchpad test of this sort?

touchpad doesn't apply really. I will add a dcheck actually.

[tdresser, 2016-06-28 17:59:24]

LGTM.

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
content/renderer/input/input_event_filter.h:87: // represent 0; otherewise. See
WebInputEventTraits::GetUniqueTouchEventId.
and should be 0 if...

It isn't clear what otherwise is referring to.

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:37: // the touch ids that need
to be acked.
This comment is a bit confusing, because we'll call this for touchpad, won't we?

Or add a DCHECK that we don't?

[dtapuska, 2016-06-28 19:51:07]

dtapuska@chromium.org changed reviewers:
+ creis@chromium.org

[dtapuska, 2016-06-28 19:51:08]

creis@chromium.org: Please rubber stamp; Tim has reviewed the real relevant
changes already.

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
content/renderer/input/input_event_filter.h:87: // represent 0; otherewise. See
WebInputEventTraits::GetUniqueTouchEventId.
On 2016/06/28 17:59:24, tdresser wrote:
> and should be 0 if...
> 
> It isn't clear what otherwise is referring to.

Done.

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2094323002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:37: // the touch ids that need
to be acked.
On 2016/06/28 17:59:24, tdresser wrote:
> This comment is a bit confusing, because we'll call this for touchpad, won't
we?
> 
> Or add a DCHECK that we don't?

Done.

[Charlie Reis, 2016-06-28 21:23:28]

RS LGTM

[dtapuska, 2016-06-29 02:33:10]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-06-29 02:33:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2094323002/#ps40001
(title: "Fix comments")

[commit-bot: I haz the power, 2016-06-29 02:33:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 03:35:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-29 03:37:15]

Description was changed from

==========
Ensure acks are sent for all blocking events.

It was possible for two touch moves that had acks to get coalesced together
and the main thread would only process one and deliver the ack for it. This
caused a situation where the hung renderer timer was getting fired because
there was an outstanding ack in the count. And this timer forced the
omnibox to not go away.

A large portion of this change (plumbing the ack_state will go away when
I land my multi-thread main_thread_event_queue design); but since this
needs to be merged back to M52 this is safest.

BUG=616991
==========

to

==========
Ensure acks are sent for all blocking events.

It was possible for two touch moves that had acks to get coalesced together
and the main thread would only process one and deliver the ack for it. This
caused a situation where the hung renderer timer was getting fired because
there was an outstanding ack in the count. And this timer forced the
omnibox to not go away.

A large portion of this change (plumbing the ack_state will go away when
I land my multi-thread main_thread_event_queue design); but since this
needs to be merged back to M52 this is safest.

BUG=616991

Committed: https://crrev.com/b08721e61d24d65dce50e40731e08231deb95932
Cr-Commit-Position: refs/heads/master@{#402703}
==========

[commit-bot: I haz the power, 2016-06-29 03:37:17]

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