Teach MainThreadEventQueue about touchmove throttling.

Teach MainThreadEventQueue about touchmove throttling.

Avoid the rAF callbacks when we are throttling touch moves. To do this
we check the last rAF time and see if there is a single touch move
in the queue that is non-cancelable (only occurs during scrolling)

BUG=642368
Review-Url: https://codereview.chromium.org/2713093005
Cr-Commit-Position: refs/heads/master@{#452985}
Committed: https://chromium.googlesource.com/chromium/src/+/97286c88bded97a7254edd950b26712fd2a616e7

[dtapuska, 2017-02-24 18:24:35]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 18:25:11]

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

[dtapuska, 2017-02-24 19:16:37]

Description was changed from

==========
Teach MainThreadEventQueue about touchmove throttling.

Avoid the rAF callbacks when we are throttling touch moves. To do this
we check the last rAF time and see if there is a single touch move
in the queue that is non-cancelable (only occurs during scrolling)

BUG=642368
==========

to

==========
Teach MainThreadEventQueue about touchmove throttling.

Avoid the rAF callbacks when we are throttling touch moves. To do this
we check the last rAF time and see if there is a single touch move
in the queue that is non-cancelable (only occurs during scrolling)

BUG=642368
==========

[dtapuska, 2017-02-24 19:16:37]

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

[dtapuska, 2017-02-24 19:18:48]

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:45: bool originallyCancelable()
const { return originally_cancelable_; }
Happy for another name. It's friday and I didn't have a better name :-)

[commit-bot: I haz the power, 2017-02-24 19:26:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 19:26:24]

Dry run: This issue passed the CQ dry run.

[tdresser, 2017-02-24 19:51:25]

This LGTM.

[dtapuska, 2017-02-24 20:18:12]

On 2017/02/24 19:51:25, tdresser wrote:
> This LGTM.

aelias@ do you care to look at this or can I TBR you since there is a very minor
change to render_widget.cc

[aelias_OOO_until_Jul13, 2017-02-24 20:19:51]

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/inpu...
content/renderer/input/input_event_filter.h:78: void ProcessRafAlignedInput(int
routing_id, double frame_time_sec) override;
Please use base::Time.

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:18: const double
kAsyncTouchMoveIntervalSec = .2;
const base::TimeDelta kAsyncTouchMoveInterval =
base::TimeDelta::FromMilliseconds(200);

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:307: if
(handle_raf_aligned_touch_input_ && shared_state_.events_.size() == 1 &&
Could you explain the reasoning why this is only needed when size is 1?

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:45: bool originallyCancelable()
const { return originally_cancelable_; }
On 2017/02/24 at 19:18:48, dtapuska wrote:
> Happy for another name. It's friday and I didn't have a better name :-)

It doesn't seem that the additional state adds any information.  And because the
caller checks the inverse, it might be better to reverse the boolean.  So I'd
suggest:

bool originallyNonblocking() const { return dispatch_type_ == EventNonBlocking
|| dispatch_type_ == ListenersNonBlockingPassive; }

[dtapuska, 2017-02-24 20:23:35]

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:307: if
(handle_raf_aligned_touch_input_ && shared_state_.events_.size() == 1 &&
On 2017/02/24 20:19:50, aelias wrote:
> Could you explain the reasoning why this is only needed when size is 1?

Because it you have anything other than one then something else needs to run.
The coalescing happens on insertion so it should always be 1 if nothing else
needs to run.

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:45: bool originallyCancelable()
const { return originally_cancelable_; }
On 2017/02/24 20:19:51, aelias wrote:
> On 2017/02/24 at 19:18:48, dtapuska wrote:
> > Happy for another name. It's friday and I didn't have a better name :-)
> 
> It doesn't seem that the additional state adds any information.  And because
the
> caller checks the inverse, it might be better to reverse the boolean.  So I'd
> suggest:
> 
> bool originallyNonblocking() const { return dispatch_type_ == EventNonBlocking
> || dispatch_type_ == ListenersNonBlockingPassive; }

Not true. dispatch_type_ is whether an ACK needs to be returned. Not that it was
non-cancelable.

You can have a non-blocking event listener but still require it to return an
ACK.

[dtapuska, 2017-02-24 20:24:19]

On 2017/02/24 20:23:35, dtapuska wrote:
>
https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
> File content/renderer/input/main_thread_event_queue.cc (right):
> 
>
https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
> content/renderer/input/main_thread_event_queue.cc:307: if
> (handle_raf_aligned_touch_input_ && shared_state_.events_.size() == 1 &&
> On 2017/02/24 20:19:50, aelias wrote:
> > Could you explain the reasoning why this is only needed when size is 1?
> 
> Because it you have anything other than one then something else needs to run.
> The coalescing happens on insertion so it should always be 1 if nothing else
> needs to run.
> 
>
https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
> File content/renderer/input/main_thread_event_queue.h (right):
> 
>
https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
> content/renderer/input/main_thread_event_queue.h:45: bool
originallyCancelable()
> const { return originally_cancelable_; }
> On 2017/02/24 20:19:51, aelias wrote:
> > On 2017/02/24 at 19:18:48, dtapuska wrote:
> > > Happy for another name. It's friday and I didn't have a better name :-)
> > 
> > It doesn't seem that the additional state adds any information.  And because
> the
> > caller checks the inverse, it might be better to reverse the boolean.  So
I'd
> > suggest:
> > 
> > bool originallyNonblocking() const { return dispatch_type_ ==
EventNonBlocking
> > || dispatch_type_ == ListenersNonBlockingPassive; }
> 
> Not true. dispatch_type_ is whether an ACK needs to be returned. Not that it
was
> non-cancelable.
> 
> You can have a non-blocking event listener but still require it to return an
> ACK.

err; see I did it there again. You can have a non-cancelable event listener but
still require it to return an ACK.

[dtapuska, 2017-02-24 20:43:15]

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/inpu...
content/renderer/input/input_event_filter.h:78: void ProcessRafAlignedInput(int
routing_id, double frame_time_sec) override;
On 2017/02/24 20:19:50, aelias wrote:
> Please use base::Time.

Done; it is a shame that cc actually returns it to the render layer in double
that really should be fixed; but that is beyond the scope of this change.

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:18: const double
kAsyncTouchMoveIntervalSec = .2;
On 2017/02/24 20:19:51, aelias wrote:
> const base::TimeDelta kAsyncTouchMoveInterval =
> base::TimeDelta::FromMilliseconds(200);

Done.

[aelias_OOO_until_Jul13, 2017-02-24 21:01:14]

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:307: if
(handle_raf_aligned_touch_input_ && shared_state_.events_.size() == 1 &&
On 2017/02/24 at 20:23:35, dtapuska wrote:
> On 2017/02/24 20:19:50, aelias wrote:
> > Could you explain the reasoning why this is only needed when size is 1?
> 
> Because it you have anything other than one then something else needs to run.
The coalescing happens on insertion so it should always be 1 if nothing else
needs to run.

OK.  Well, this control flow seems like it could be less hairy, the duplication
of the emplace_back() line has a bad code smell in particular.

How about the following instead.  It's mostly equivalent to what you wrote
except that async touchmoves at the end of a multi-element queue are treated the
same as async touchmoves in an originally size-1 queue, which seems appropriate.

while (!shared_state_.events_.empty()) {
  if (!IsRafAlignedEvent(shared_state_.events_.front()->event()))
    break;
  if (IsAsyncTouchMove(shared_state_.events_.front()) &&
handle_raf_aligned_touch_input_) {
    if (shared_state_.events_.size() == 1 && frame_time_sec <
shared_state_.last_async_touch_move_timestamp_ + kAsyncTouchMoveIntervalSec)
      break;
    shared_state_.last_async_touch_move_timestamp_ = frame_time_sec;
  } 

  events_to_process.emplace_back(shared_state_.events_.Pop());
}

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:45: bool originallyCancelable()
const { return originally_cancelable_; }
On 2017/02/24 at 20:23:35, dtapuska wrote:
> Not true. dispatch_type_ is whether an ACK needs to be returned. Not that it
was non-cancelable.
> 
> You can have a non-blocking event listener but still require it to return an
ACK.

OK.  If I understand correctly, this is a problem with the name but not a bug in
the implementation I proposed.  I'd still prefer not storing a separate boolean:

bool originallyCancelable() const { return dispatch_type_ != EventNonBlocking
&& dispatch_type_ != ListenersNonBlockingPassive; }

[dtapuska, 2017-02-24 21:29:41]

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:307: if
(handle_raf_aligned_touch_input_ && shared_state_.events_.size() == 1 &&
On 2017/02/24 21:01:14, aelias wrote:
> On 2017/02/24 at 20:23:35, dtapuska wrote:
> > On 2017/02/24 20:19:50, aelias wrote:
> > > Could you explain the reasoning why this is only needed when size is 1?
> > 
> > Because it you have anything other than one then something else needs to
run.
> The coalescing happens on insertion so it should always be 1 if nothing else
> needs to run.
> 
> OK.  Well, this control flow seems like it could be less hairy, the
duplication
> of the emplace_back() line has a bad code smell in particular.
> 
> How about the following instead.  It's mostly equivalent to what you wrote
> except that async touchmoves at the end of a multi-element queue are treated
the
> same as async touchmoves in an originally size-1 queue, which seems
appropriate.
> 
> while (!shared_state_.events_.empty()) {
>   if (!IsRafAlignedEvent(shared_state_.events_.front()->event()))
>     break;
>   if (IsAsyncTouchMove(shared_state_.events_.front()) &&
> handle_raf_aligned_touch_input_) {
>     if (shared_state_.events_.size() == 1 && frame_time_sec <
> shared_state_.last_async_touch_move_timestamp_ + kAsyncTouchMoveIntervalSec)
>       break;
>     shared_state_.last_async_touch_move_timestamp_ = frame_time_sec;
>   } 
> 
>   events_to_process.emplace_back(shared_state_.events_.Pop());
> }

Done.

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

https://codereview.chromium.org/2713093005/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.h:45: bool originallyCancelable()
const { return originally_cancelable_; }
On 2017/02/24 21:01:14, aelias wrote:
> On 2017/02/24 at 20:23:35, dtapuska wrote:
> > Not true. dispatch_type_ is whether an ACK needs to be returned. Not that it
> was non-cancelable.
> > 
> > You can have a non-blocking event listener but still require it to return an
> ACK.
> 
> OK.  If I understand correctly, this is a problem with the name but not a bug
in
> the implementation I proposed.  I'd still prefer not storing a separate
boolean:
> 
> bool originallyCancelable() const { return dispatch_type_ != EventNonBlocking
> && dispatch_type_ != ListenersNonBlockingPassive; }

These dispatch_types_ are of different enums. We really should rename the value
in the WebInputEvent

[aelias_OOO_until_Jul13, 2017-02-24 21:34:54]

lgtm

[dtapuska, 2017-02-24 21:37:23]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-02-24 21:37:24]

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/2713093005/#ps40001
(title: "Roll if/else condition together")

[commit-bot: I haz the power, 2017-02-24 21:37:59]

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, 2017-02-24 23:14:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487972243875470,
"parent_rev": "f1d5292e67790a384c212c6e6d25f1655591aebf", "commit_rev":
"97286c88bded97a7254edd950b26712fd2a616e7"}

[commit-bot: I haz the power, 2017-02-24 23:15:25]

Description was changed from

==========
Teach MainThreadEventQueue about touchmove throttling.

Avoid the rAF callbacks when we are throttling touch moves. To do this
we check the last rAF time and see if there is a single touch move
in the queue that is non-cancelable (only occurs during scrolling)

BUG=642368
==========

to

==========
Teach MainThreadEventQueue about touchmove throttling.

Avoid the rAF callbacks when we are throttling touch moves. To do this
we check the last rAF time and see if there is a single touch move
in the queue that is non-cancelable (only occurs during scrolling)

BUG=642368

Review-Url: https://codereview.chromium.org/2713093005
Cr-Commit-Position: refs/heads/master@{#452985}
Committed:
https://chromium.googlesource.com/chromium/src/+/97286c88bded97a7254edd950b26...
==========

[commit-bot: I haz the power, 2017-02-24 23:15:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/97286c88bded97a7254edd950b26...