Fix priority of TaskQueue::QueueType::COMPOSITOR on touch events

Fix priority of TaskQueue::QueueType::COMPOSITOR on touch events

RendererSchedulerImpl::UpdatePolicyLocked attempts to increase priority of
TaskQueue::QueueType::COMPOSITOR on touch events. To do that, it asks UserModel
if it expects a gesture. UserModel starts expecting a gesture when its method
DidStartProcessingInputEvent is called. The method DidStartProcessingInputEvent
was not called for blink::WebInputEvent::TouchStart, even though the method
clearly expects the event (compares its input param with it), and the event
signals of a gesture start.

The method UserModel::DidStartProcessingInputEvent should propogate
blink::WebInputEvent::TouchStart by next call stack:
    InputHandlerManager::DidHandleInputEventAndOverscroll =>
    RendererSchedulerImpl::DidHandleInputEventOnCompositorThread =>
    RendererSchedulerImpl::UpdateForInputEventOnCompositorThread =>
    UserModel::DidStartProcessingInputEvent

But the top method (DidHandleInputEventAndOverscroll) doesn't propogate event
blink::WebInputEvent::TouchStart when it is received.
The method doesn't propogate the event because a switch-case in it
doesn't handle InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING
(which is generated for blink::WebInputEvent::TouchStart).

The switch-case in InputHandlerManager::DidHandleInputEventAndOverscroll was
created by commit 7819e255a595, and the not-handled-state
(InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING) didn't
exist back then - the switch-case handled all meaningful elements of
InputEventAckState back then (the elements that meant that the touch event
was not ignored).
When InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING was created
(0bd451acef1d), a case for it was not added to the switch-case, which causes
blink::WebInputEvent::TouchStart to be ignored by
InputHandlerManager::DidHandleInputEventAndOverscroll.

BUG=705884

[blazern, 2017-03-28 08:15:04]

The CQ bit was checked by blazern@yandex-team.ru

[blazern, 2017-03-28 08:15:09]

The CQ bit was unchecked by blazern@yandex-team.ru

[blazern, 2017-03-28 08:23:12]

Description was changed from

==========
Fix priority of TaskQueue::QueueType::COMPOSITOR on touch events

RendererSchedulerImpl::UpdatePolicyLocked attempts to increase priority of
TaskQueue::QueueType::COMPOSITOR on touch events. To do that, it asks UserModel
if it expects a gesture. UserModel starts expecting a gesture when its method
DidStartProcessingInputEvent is called. The method DidStartProcessingInputEvent
was not called for blink::WebInputEvent::TouchStart, even though the method
clearly expects the event (compares its input param with it), and the event
signals of a gesture start.

The method UserModel::DidStartProcessingInputEvent should propogate
blink::WebInputEvent::TouchStart by next call stack:
    InputHandlerManager::DidHandleInputEventAndOverscroll =>
    RendererSchedulerImpl::DidHandleInputEventOnCompositorThread =>
    RendererSchedulerImpl::UpdateForInputEventOnCompositorThread =>
    UserModel::DidStartProcessingInputEvent

But the top method (DidHandleInputEventAndOverscroll) doesn't propogate event
blink::WebInputEvent::TouchStart when it is received.
The method doesn't propogate the event because a switch-case in it
doesn't handle InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING
(which is generated for blink::WebInputEvent::TouchStart).

The switch-case in InputHandlerManager::DidHandleInputEventAndOverscroll was
created by commit 7819e255a595, and the not-handled-state
(InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING) didn't
exist back then - the switch-case handled all meaningful elements of
InputEventAckState back then (the elements that meant that the touch event
was not ignored).
When InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING was created
(0bd451acef1d), a case for it was not added to the switch-case, which causes
blink::WebInputEvent::TouchStart to be ignored by
InputHandlerManager::DidHandleInputEventAndOverscroll.

BUG=705884
==========

to

==========
Fix priority of TaskQueue::QueueType::COMPOSITOR on touch events

RendererSchedulerImpl::UpdatePolicyLocked attempts to increase priority of
TaskQueue::QueueType::COMPOSITOR on touch events. To do that, it asks UserModel
if it expects a gesture. UserModel starts expecting a gesture when its method
DidStartProcessingInputEvent is called. The method DidStartProcessingInputEvent
was not called for blink::WebInputEvent::TouchStart, even though the method
clearly expects the event (compares its input param with it), and the event
signals of a gesture start.

The method UserModel::DidStartProcessingInputEvent should propogate
blink::WebInputEvent::TouchStart by next call stack:
    InputHandlerManager::DidHandleInputEventAndOverscroll =>
    RendererSchedulerImpl::DidHandleInputEventOnCompositorThread =>
    RendererSchedulerImpl::UpdateForInputEventOnCompositorThread =>
    UserModel::DidStartProcessingInputEvent

But the top method (DidHandleInputEventAndOverscroll) doesn't propogate event
blink::WebInputEvent::TouchStart when it is received.
The method doesn't propogate the event because a switch-case in it
doesn't handle InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING
(which is generated for blink::WebInputEvent::TouchStart).

The switch-case in InputHandlerManager::DidHandleInputEventAndOverscroll was
created by commit 7819e255a595, and the not-handled-state
(InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING) didn't
exist back then - the switch-case handled all meaningful elements of
InputEventAckState back then (the elements that meant that the touch event
was not ignored).
When InputEventAckState::INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING was created
(0bd451acef1d), a case for it was not added to the switch-case, which causes
blink::WebInputEvent::TouchStart to be ignored by
InputHandlerManager::DidHandleInputEventAndOverscroll.

BUG=705884
==========

[blazern, 2017-03-28 08:23:12]

blazern@yandex-team.ru changed reviewers:
+ dtapuska@chromium.org, skyostil@chromium.org

[dtapuska, 2017-03-28 13:33:03]

On 2017/03/28 08:26:27, blazern wrote:

Not lgtm; this is explicitly on purpose. This will cause all gains due to the
passive event listeners to be lost. The idea is that we don't really tell the
scheduler if we are sending a passive event to the main thread because we don't
want to continue having the compositor have priority over the main thread.

[Sami, 2017-03-28 13:42:35]

That's right, we don't need to worry too much about the latency of non-blocking
input events on the main thread.

Do you have an example page where this caused a performance problem?

[dtapuska, 2017-03-28 13:49:37]

On 2017/03/28 13:42:35, Sami wrote:
> That's right, we don't need to worry too much about the latency of
non-blocking
> input events on the main thread.
> 
> Do you have an example page where this caused a performance problem?

That being said I think we need to tell the scheduler about these events so it
isn't illogical like this. But it shouldn't bump priorities based on it. ie;
there is a flaw that whenever the event is handled on the main thread it
decrements the count pending count even if it was sent as non-blocking.

[Sami, 2017-03-28 13:51:27]

On 2017/03/28 13:49:37, dtapuska wrote:
> That being said I think we need to tell the scheduler about these events so it
> isn't illogical like this. But it shouldn't bump priorities based on it. ie;
> there is a flaw that whenever the event is handled on the main thread it
> decrements the count pending count even if it was sent as non-blocking.

Interesting, should we pass the blocking mode as a part of the input event?
(Assuming it isn't already)

[blazern, 2017-03-29 06:38:14]

On 2017/03/28 13:42:35, Sami wrote:
> That's right, we don't need to worry too much about the latency of
non-blocking
> input events on the main thread.
> 
> Do you have an example page where this caused a performance problem?

Yes, I do.
The example page is news-site http://www.rbc.ru/.

To reproduce the performance problem one should:
1. Build the browser in debug mode (so it would be slower and the problem would
more likely to appear).
2. Start the browser with flag "--process-per-site" (so that subpages of a page
would load in the same process).
3. Open http://www.rbc.ru/.
4. Scroll down news articles list.
5. Start opening the articles in separate tabs by longclicking them and
selecting "Open in new tab".
After one or two opened tabs the render process stops reacting to long clicks
for 5-10 seconds.
It's important to not scroll when performing the 5th step (openning multiple
articles), as it seems that scroll increases compositor's priority and as a
result long clicks are handled fast.

A screenrecord of reproducing: https://youtu.be/WvfJgs_xkRM
A screenrecord of reproducing attempt after applying the proposed patch:
https://youtu.be/RqyTRTSn29A