scheduler: Detect single event gestures correctly

scheduler: Detect single event gestures correctly

When a single event gesture such as a press (i.e., a single touchstart
event) is observed, the scheduler would previously wait for two
subsequent touchmove events before deciding the gesture was established
and stop blocking tasks unrelated to input handling. The problem was
that with single event gestures, these additional events are never sent,
so the scheduler would remain in 'touchstart' mode for 100ms, possibly
causing important tasks to get delayed.

This patch fixes the issue by telling the scheduler how the touchstart
event was handled. If the event handler calls preventDefault() on the
event, the scheduler can immediately conclude that the gesture is
handled by the main thread without waiting for any further events. If
preventDefault() is not called, we maintain the previous behavior.

BUG=639300
Review-Url: https://codereview.chromium.org/2661443003
Cr-Commit-Position: refs/heads/master@{#446921}
Committed: https://chromium.googlesource.com/chromium/src/+/cff6288fd965f274205ca6ab9cded462ba204dc9

[Sami, 2017-01-27 12:49:12]

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

[commit-bot: I haz the power, 2017-01-27 12:49:25]

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

[Sami, 2017-01-27 12:49:42]

Description was changed from

==========
scheduler: Detect single event gestures correctly

When a single event gesture such as a press (i.e., a single touchstart
event) is observed, the scheduler would previously wait for two
subsequent touchmove events before deciding the gesture was established
and stop blocking tasks unrelated to input handling. The problem was
that with single event gestures, these additional events are never sent,
so the scheduler would remain in 'touchstart' mode for 100ms, possibly
causing important tasks to get delayed.

This patch fixes the issue by telling the scheduler how the touchstart
event was handled. If the event handler calls preventDefault() on the
event, the scheduler can immediately conclude that the gesture is
handled by the main thread without waiting for any further events. If
preventDefault() is not called, we maintain the previous behavior.

BUG=639300
==========

to

==========
scheduler: Detect single event gestures correctly

When a single event gesture such as a press (i.e., a single touchstart
event) is observed, the scheduler would previously wait for two
subsequent touchmove events before deciding the gesture was established
and stop blocking tasks unrelated to input handling. The problem was
that with single event gestures, these additional events are never sent,
so the scheduler would remain in 'touchstart' mode for 100ms, possibly
causing important tasks to get delayed.

This patch fixes the issue by telling the scheduler how the touchstart
event was handled. If the event handler calls preventDefault() on the
event, the scheduler can immediately conclude that the gesture is
handled by the main thread without waiting for any further events. If
preventDefault() is not called, we maintain the previous behavior.

BUG=639300
==========

[Sami, 2017-01-27 12:49:42]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org, dtapuska@chromium.org

[Sami, 2017-01-27 12:50:23]

Adding dtapuska@ for content/renderer/input/ and alexclarke@ for
third_party/WebKit/Source/platform/scheduler/ -- PTAL.

[alex clarke (OOO till 29th), 2017-01-27 13:25:44]

lgtm

https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:996:
EXPECT_THAT(run_order,
Maybe add a comment explaining the significance of letting the loading task run?

https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1019:
EXPECT_THAT(run_order,
And maybe a comment explaining why it was blocked here?

[commit-bot: I haz the power, 2017-01-27 14:15:51]

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

[commit-bot: I haz the power, 2017-01-27 14:15:51]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dtapuska, 2017-01-27 14:27:33]

On 2017/01/27 13:25:44, alex clarke wrote:
> lgtm
> 
>
https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
> File
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:996:
> EXPECT_THAT(run_order,
> Maybe add a comment explaining the significance of letting the loading task
run?
> 
>
https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1019:
> EXPECT_THAT(run_order,
> And maybe a comment explaining why it was blocked here?

What about the touch-action response? Are you correctly dealing with that?

[Sami, 2017-01-27 14:50:54]

> What about the touch-action response? Are you correctly dealing with that?

I'm not sure what you mean? Do you mean tasks that are triggered as a side
effect of touch handling? Those are handled in the same way as before according
to the scheduler policy. The bug here was that we weren't setting the right
policy to begin with.

https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:996:
EXPECT_THAT(run_order,
On 2017/01/27 13:25:44, alex clarke wrote:
> Maybe add a comment explaining the significance of letting the loading task
run?

Done.

https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1019:
EXPECT_THAT(run_order,
On 2017/01/27 13:25:44, alex clarke wrote:
> And maybe a comment explaining why it was blocked here?

Done.

[dtapuska, 2017-01-27 15:03:31]

On 2017/01/27 14:50:54, Sami wrote:
> > What about the touch-action response? Are you correctly dealing with that?
> 
> I'm not sure what you mean? Do you mean tasks that are triggered as a side
> effect of touch handling? Those are handled in the same way as before
according
> to the scheduler policy. The bug here was that we weren't setting the right
> policy to begin with.
> 
>
https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
> File
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:996:
> EXPECT_THAT(run_order,
> On 2017/01/27 13:25:44, alex clarke wrote:
> > Maybe add a comment explaining the significance of letting the loading task
> run?
> 
> Done.
> 
>
https://codereview.chromium.org/2661443003/diff/1/third_party/WebKit/Source/p...
>
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1019:
> EXPECT_THAT(run_order,
> On 2017/01/27 13:25:44, alex clarke wrote:
> > And maybe a comment explaining why it was blocked here?
> 
> Done.

The default gesture can get prevented with a touch-action: none as well.   ie;
calling preventDefault() is equivalent to setting a touch-action: none on the
item.

[Sami, 2017-01-27 16:15:58]

On 2017/01/27 15:03:31, dtapuska wrote:
> The default gesture can get prevented with a touch-action: none as well.   ie;
> calling preventDefault() is equivalent to setting a touch-action: none on the
> item.

Ah, I see. Looks like we'll need to plumb that somehow to the scheduler too --
maybe via RenderWidget::setTouchAction(). I'd prefer to do that in a follow-up
patch since that change will be mostly orthogonal to this one -- WDYT? Filed
crbug.com/686110.

[dtapuska, 2017-01-27 16:16:53]

On 2017/01/27 16:15:58, Sami wrote:
> On 2017/01/27 15:03:31, dtapuska wrote:
> > The default gesture can get prevented with a touch-action: none as well.  
ie;
> > calling preventDefault() is equivalent to setting a touch-action: none on
the
> > item.
> 
> Ah, I see. Looks like we'll need to plumb that somehow to the scheduler too --
> maybe via RenderWidget::setTouchAction(). I'd prefer to do that in a follow-up
> patch since that change will be mostly orthogonal to this one -- WDYT? Filed
> crbug.com/686110.

ya this is lgtm for now

[Sami, 2017-01-27 18:39:04]

skyostil@chromium.org changed reviewers:
+ kinuko@chromium.org

[Sami, 2017-01-27 18:39:20]

Thanks! kinuko@, could you check content/renderer please?

[kinuko, 2017-01-28 02:24:17]

lgtm

[Sami, 2017-01-28 11:37:49]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2017-01-28 11:37:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2661443003/#ps20001
(title: "Review comments")

[commit-bot: I haz the power, 2017-01-28 11:37:53]

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-01-28 13:18:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485603469044980,
"parent_rev": "90cdf584494922804c8d0c81a83bcad271645659", "commit_rev":
"cff6288fd965f274205ca6ab9cded462ba204dc9"}

[commit-bot: I haz the power, 2017-01-28 13:18:45]

Description was changed from

==========
scheduler: Detect single event gestures correctly

When a single event gesture such as a press (i.e., a single touchstart
event) is observed, the scheduler would previously wait for two
subsequent touchmove events before deciding the gesture was established
and stop blocking tasks unrelated to input handling. The problem was
that with single event gestures, these additional events are never sent,
so the scheduler would remain in 'touchstart' mode for 100ms, possibly
causing important tasks to get delayed.

This patch fixes the issue by telling the scheduler how the touchstart
event was handled. If the event handler calls preventDefault() on the
event, the scheduler can immediately conclude that the gesture is
handled by the main thread without waiting for any further events. If
preventDefault() is not called, we maintain the previous behavior.

BUG=639300
==========

to

==========
scheduler: Detect single event gestures correctly

When a single event gesture such as a press (i.e., a single touchstart
event) is observed, the scheduler would previously wait for two
subsequent touchmove events before deciding the gesture was established
and stop blocking tasks unrelated to input handling. The problem was
that with single event gestures, these additional events are never sent,
so the scheduler would remain in 'touchstart' mode for 100ms, possibly
causing important tasks to get delayed.

This patch fixes the issue by telling the scheduler how the touchstart
event was handled. If the event handler calls preventDefault() on the
event, the scheduler can immediately conclude that the gesture is
handled by the main thread without waiting for any further events. If
preventDefault() is not called, we maintain the previous behavior.

BUG=639300

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

[commit-bot: I haz the power, 2017-01-28 13:18:47]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/cff6288fd965f274205ca6ab9cde...