Allow events in queue to coalesce while executing other events.

Allow events in queue to coalesce while executing other events.

The code removed all events from the queue at the start which is flawed
because a slow handler could cause another event not to be coalesced.
This was explicitly tested in the browser test that is failing sometimes
now.

BUG=707491, 674301
Review-Url: https://codereview.chromium.org/2793963002
Cr-Commit-Position: refs/heads/master@{#461843}
Committed: https://chromium.googlesource.com/chromium/src/+/b1e592331aa12efbde84280e8b4e9248a7360c9d

[dtapuska, 2017-04-03 14:24:15]

Description was changed from

==========
Allow events in queue to coalesce while executing other events.

The code removed all events from the queue at the start which is flawed
because a slow handler could cause another event not to be coalesced.
This was explicitly tested in the browser test that is failing sometimes
now.

BUG=707491,674301
==========

to

==========
Allow events in queue to coalesce while executing other events.

The code removed all events from the queue at the start which is flawed
because a slow handler could cause another event not to be coalesced.
This was explicitly tested in the browser test that is failing sometimes
now.

BUG=707491,674301
==========

[dtapuska, 2017-04-03 14:24:15]

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

[dtapuska, 2017-04-03 14:24:24]

PTAL

[mustaq, 2017-04-04 14:59:22]

As we discussed offline...

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

https://codereview.chromium.org/2793963002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:409:
shared_state_.sent_main_frame_request_ = false;
Just spotted this nit from before: |shared_state_.sent_main_frame_reuqest_| is
always false here, so Lines 408~409 could be replaced by |needs_main_frame =
true;|;

https://codereview.chromium.org/2793963002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:434: for (size_t i = 0; i <
shared_state_.events_.size(); ++i) {
The use of .size() here seems suspicious: since we only care about first
n=|queue_size_at_start| events, events queued after the locked state shouldn't
trigger early bail out from the |while| loop. Am I right?

If so, shouldn't we compute |all_raf_aligned| in locked state above?

[dtapuska, 2017-04-04 15:27:09]

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

[commit-bot: I haz the power, 2017-04-04 15:27:42]

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

[dtapuska, 2017-04-04 15:27:45]

PTAL

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

https://codereview.chromium.org/2793963002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:409:
shared_state_.sent_main_frame_request_ = false;
On 2017/04/04 14:59:22, mustaq wrote:
> Just spotted this nit from before: |shared_state_.sent_main_frame_reuqest_| is
> always false here, so Lines 408~409 could be replaced by |needs_main_frame =
> true;|;

Done.

https://codereview.chromium.org/2793963002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:434: for (size_t i = 0; i <
shared_state_.events_.size(); ++i) {
On 2017/04/04 14:59:22, mustaq wrote:
> The use of .size() here seems suspicious: since we only care about first
> n=|queue_size_at_start| events, events queued after the locked state shouldn't
> trigger early bail out from the |while| loop. Am I right?
> 
> If so, shouldn't we compute |all_raf_aligned| in locked state above?

Done.

[mustaq, 2017-04-04 16:00:23]

LGTM with nits:

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

https://codereview.chromium.org/2793963002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:426: // Look at the tail of
the queue and determine how many rAF
Nit: we are not exactly "counting" rAF aligned events. May be:
// Don't process any rAF aligned events at the tail of the queue.

https://codereview.chromium.org/2793963002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:428: for (size_t i =
shared_state_.events_.size(); i > 0; --i) {
Nit: perhaps replace with a 2+ line |while| loop?

while(IsRafAlignedEvent(shared_state_.events_.at(events_to_process - 1)))
  events_to_process--;

[commit-bot: I haz the power, 2017-04-04 16:21:42]

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

[commit-bot: I haz the power, 2017-04-04 16:21:42]

Dry run: This issue passed the CQ dry run.

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

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

https://codereview.chromium.org/2793963002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:426: // Look at the tail of
the queue and determine how many rAF
On 2017/04/04 16:00:22, mustaq wrote:
> Nit: we are not exactly "counting" rAF aligned events. May be:
> // Don't process any rAF aligned events at the tail of the queue.

Done.

https://codereview.chromium.org/2793963002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:428: for (size_t i =
shared_state_.events_.size(); i > 0; --i) {
On 2017/04/04 16:00:23, mustaq wrote:
> Nit: perhaps replace with a 2+ line |while| loop?
> 
> while(IsRafAlignedEvent(shared_state_.events_.at(events_to_process - 1)))
>   events_to_process--;

Done.

[dtapuska, 2017-04-04 20:02:23]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-04-04 20:02:23]

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

[commit-bot: I haz the power, 2017-04-04 20:02:54]

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-04-04 21:22:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491336143381270,
"parent_rev": "8e43085f1a7ff3f2ad09d7699e5c7cd81118e7fb", "commit_rev":
"b1e592331aa12efbde84280e8b4e9248a7360c9d"}

[commit-bot: I haz the power, 2017-04-04 21:22:52]

Description was changed from

==========
Allow events in queue to coalesce while executing other events.

The code removed all events from the queue at the start which is flawed
because a slow handler could cause another event not to be coalesced.
This was explicitly tested in the browser test that is failing sometimes
now.

BUG=707491,674301
==========

to

==========
Allow events in queue to coalesce while executing other events.

The code removed all events from the queue at the start which is flawed
because a slow handler could cause another event not to be coalesced.
This was explicitly tested in the browser test that is failing sometimes
now.

BUG=707491,674301

Review-Url: https://codereview.chromium.org/2793963002
Cr-Commit-Position: refs/heads/master@{#461843}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1e592331aa12efbde84280e8b4e...
==========

[commit-bot: I haz the power, 2017-04-04 21:22:53]

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