[POC; Not for Review] Don't use PostTask queueing between compositor and main thread.

Don't use PostTask queueing between compositor and main thread.

Instead put the events in the main thread event queue where they can
be coalesced.

Don't look at this code too much it is only proof of concept. I expect I
could clean up the code substantially by using 1 underlying queue
instead of 3.

BUG=599152

[dtapuska, 2016-05-25 18:57:20]

Description was changed from

==========
Don't use PostTask queueing between compositor and main thread.

Instead put the events in the main thread event queue where they can
be coalesced.

Don't look at this code too much it is only proof of concept. I expect I
could clean up the code substantially by using 1 underlying queue
instead of 3.

BUG=599152
==========

to

==========
Don't use PostTask queueing between compositor and main thread.

Instead put the events in the main thread event queue where they can
be coalesced.

Don't look at this code too much it is only proof of concept. I expect I
could clean up the code substantially by using 1 underlying queue
instead of 3.

BUG=599152
==========

[dtapuska, 2016-05-25 18:57:20]

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

[dtapuska, 2016-05-25 18:58:13]

On 2016/05/25 18:57:30, dtapuska wrote:

Feel free to harp on this design; I'm sure much could be cleaned up though.

[aelias_OOO_until_Jul13, 2016-05-25 22:16:01]

Seems OK.  I like that this also would make it easier to annotate the input
events with their resolved layer position, instead of having Blink do its own
viewport-based hit testing.

There is also coalescing logic in
content/browser/renderer_host/input/touch_event_queue.cc.  Is the plan to
eventually do all the coalescing on the renderer side, or is there a reason why
we need to have both?

[tdresser, 2016-05-30 14:05:32]

High level approach LGTM.

https://codereview.chromium.org/2007413002/diff/20001/content/common/input/we...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/2007413002/diff/20001/content/common/input/we...
content/common/input/web_input_event_traits.cc:493: case
WebInputEvent::MouseMove:
Will we still coalesce these correctly now? Or will we end up sending every
mousemove all the way to the renderer?

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

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:80: void
MainThreadEventQueue::EventHandled(blink::WebInputEvent::Type type) {}
So we'd get rid of this completely?

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:96: if
(!wheel_events_.empty())
Should this case ever occur?

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:100: if (event) {
Should this ever not be true?

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

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:70: // from the compositor
thread.
Don't forget to change this comment.

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:143: base::Callback<void(const
IPC::Message&)> main_listener_;
Eventually we'll want a couple ThreadChecker's in here.

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

[Sami, 2016-05-31 16:49:34]

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
File content/renderer/input/input_event_filter.cc (left):

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/input_event_filter.cc:242:
main_task_runner_->PostTask(FROM_HERE, base::Bind(main_listener_, new_msg));
Some of the logic in the renderer scheduler looks at pending work in the
input/compositing task queue (mainly [1]). If we're no longer posting tasks
there, that might cause us to miss some cases.

I *think* [1] isn't that useful anymore since most users have been converted
from yielding to using individual tasks. As long as we still call notifications
like DidHandleInputEventOnCompositorThread and DidHandleInputEventOnMainThread
the important cases should be covered.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul...

[dtapuska, 2016-06-01 18:22:54]

https://codereview.chromium.org/2007413002/diff/20001/content/common/input/we...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/2007413002/diff/20001/content/common/input/we...
content/common/input/web_input_event_traits.cc:493: case
WebInputEvent::MouseMove:
On 2016/05/30 14:05:32, tdresser wrote:
> Will we still coalesce these correctly now? Or will we end up sending every
> mousemove all the way to the renderer?

Mouse moves will go to the compositor and will be unthrottled. Essentially no
coalescing in the browser anymore.

Alternatively I could change this so it does block but mark them as handled in
the compositor (that is likely better).

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
File content/renderer/input/input_event_filter.cc (left):

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/input_event_filter.cc:242:
main_task_runner_->PostTask(FROM_HERE, base::Bind(main_listener_, new_msg));
On 2016/05/31 16:49:34, Sami wrote:
> Some of the logic in the renderer scheduler looks at pending work in the
> input/compositing task queue (mainly [1]). If we're no longer posting tasks
> there, that might cause us to miss some cases.
> 
> I *think* [1] isn't that useful anymore since most users have been converted
> from yielding to using individual tasks. As long as we still call
notifications
> like DidHandleInputEventOnCompositorThread and DidHandleInputEventOnMainThread
> the important cases should be covered.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul...

We still are posting tasks; it just occurs at a different spot.

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

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:80: void
MainThreadEventQueue::EventHandled(blink::WebInputEvent::Type type) {}
On 2016/05/30 14:05:32, tdresser wrote:
> So we'd get rid of this completely?

Yes

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:96: if
(!wheel_events_.empty())
On 2016/05/30 14:05:32, tdresser wrote:
> Should this case ever occur?

No it shouldn't can add dcheck

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:100: if (event) {
On 2016/05/30 14:05:32, tdresser wrote:
> Should this ever not be true?

No it shouldn't

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

https://codereview.chromium.org/2007413002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:143: base::Callback<void(const
IPC::Message&)> main_listener_;
On 2016/05/30 14:05:32, tdresser wrote:
> Eventually we'll want a couple ThreadChecker's in here.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

Acknowledged.

[Rick Byers, 2016-06-01 18:27:37]

Yes I think this approach is a good step in the right direction.  In particular,
it makes sense to want to use the current co-ordinates when dispatching an input
event, rather than whatever the co-ordinates were when the task got posted -
will decrease input latency when the main thread is congested.

Ultimately we want to move to a model where input events are integrated with the
document lifecycle (i.e. sent in the context of WebViewImpl::beginFrame like
scroll events and raf callbacks are).

Doing that would probably make this CL simpler - prevent the need for "handle
input now" tasks (the compositor would just need to know to schedule a main
frame when there is pending input).  However it involves some extra complexity. 
In particular we'd probably want to interpolate/extrapolate the
positions/timestamps (like BufferedInputRouter and the android framework do). 
It also potentially hurts latency in some cases (probably only want that if we
can disable the Android Framework's own input event queue).

So I agree that something like this patch is the right next step, improving some
scenarios without adding new problems / risks.

[dtapuska, 2016-07-19 19:46:44]

On 2016/06/01 18:27:37, Rick Byers (out until Aug 1) wrote:
> Yes I think this approach is a good step in the right direction.  In
particular,
> it makes sense to want to use the current co-ordinates when dispatching an
input
> event, rather than whatever the co-ordinates were when the task got posted -
> will decrease input latency when the main thread is congested.
> 
> Ultimately we want to move to a model where input events are integrated with
the
> document lifecycle (i.e. sent in the context of WebViewImpl::beginFrame like
> scroll events and raf callbacks are).
> 
> Doing that would probably make this CL simpler - prevent the need for "handle
> input now" tasks (the compositor would just need to know to schedule a main
> frame when there is pending input).  However it involves some extra
complexity. 
> In particular we'd probably want to interpolate/extrapolate the
> positions/timestamps (like BufferedInputRouter and the android framework do). 
> It also potentially hurts latency in some cases (probably only want that if we
> can disable the Android Framework's own input event queue).
> 
> So I agree that something like this patch is the right next step, improving
some
> scenarios without adding new problems / risks.

Follow on change is here: https://codereview.chromium.org/2162143002/