Implement Main Thread RAF Aligned Input

Introduce the concept of RAF aligned input events to the MainThreadEventQueue.
Currently behind a feature flag and is disabled.

Short Design Doc:

https://docs.google.com/document/d/1aM9AqyYuceRHOmsJZXFKcgi_D4jvFAymYvI36OEwIpI/edit?usp=sharing

BUG=625693
Committed: https://crrev.com/3d5624d3a3aa88e0fafe5bd09b5725b1b3f25d67
Cr-Commit-Position: refs/heads/master@{#415099}

[dtapuska, 2016-07-20 15:13:23]

Description was changed from

==========
A simple prototype of main thread raf aligned input.

BUG=625693
==========

to

==========
A simple prototype of main thread raf aligned input.

BUG=625693
==========

[dtapuska, 2016-07-20 15:13:23]

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

[dtapuska, 2016-07-20 15:14:22]

Tim, let me know what you think.

I think this is missing the ability to schedule a begin main frame if we have
input but not a main frame scheduled.

But for my simple tests it seemed to be working ok. I don't know why though.

[tdresser, 2016-07-20 21:43:09]

High level looks good. Let me know if I should do a more thorough review.

I noted a few nits as I went through.

I think we want to view this as BeginFrame aligned (or rAF aligned), not VSync
aligned. For example in the BeginRetroFrame case, these could be pretty
different.

https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
File content/common/input/web_input_event_queue.h (right):

https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
content/common/input/web_input_event_queue.h:59: const std::unique_ptr<T>& Top()
{
Why Top instead of front?

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

https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/inpu...
content/renderer/input/input_event_filter.h:73: void
ProcessVSyncAlignedInput(int routing_id) override;
I think we want to align with rAF more than we want to align with vsync.
Normally they're the same, but they won't always be (if I understand correctly).

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

https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:12: const size_t
kMaxEventsPerVSyncEvent = 10;
Add a comment about this constant - why do we need a max here?

https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:13: bool
isVSyncAlignedEvent(const std::unique_ptr<EventWithDispatchType>& event) {
I'd name this something like eventCanBeRafAligned.

Or maybe something like "isContinousEvent", as opposed to discrete events?

https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:157: LOG(ERROR) << "Processed
" << i << " events in RAF";
Remember to remove this before landing (and make the for loop more normal).

https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue.cc:168: bool isVsynced =
isVSyncAlignedEvent(event);
Inconsistent casing here.

[dtapuska, 2016-08-11 21:02:18]

Description was changed from

==========
A simple prototype of main thread raf aligned input.

BUG=625693
==========

to

==========
A simple prototype of main thread raf aligned input.

Short Design Doc:

https://docs.google.com/document/d/1aM9AqyYuceRHOmsJZXFKcgi_D4jvFAymYvI36OEwI...

BUG=625693
==========

[dtapuska, 2016-08-11 21:07:23]

On 2016/07/20 21:43:09, tdresser wrote:
> High level looks good. Let me know if I should do a more thorough review.
> 
> I noted a few nits as I went through.
> 
> I think we want to view this as BeginFrame aligned (or rAF aligned), not VSync
> aligned. For example in the BeginRetroFrame case, these could be pretty
> different.
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
> File content/common/input/web_input_event_queue.h (right):
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
> content/common/input/web_input_event_queue.h:59: const std::unique_ptr<T>&
Top()
> {
> Why Top instead of front?
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/inpu...
> File content/renderer/input/input_event_filter.h (right):
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/inpu...
> content/renderer/input/input_event_filter.h:73: void
> ProcessVSyncAlignedInput(int routing_id) override;
> I think we want to align with rAF more than we want to align with vsync.
> Normally they're the same, but they won't always be (if I understand
correctly).
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> File content/renderer/input/main_thread_event_queue.cc (right):
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> content/renderer/input/main_thread_event_queue.cc:12: const size_t
> kMaxEventsPerVSyncEvent = 10;
> Add a comment about this constant - why do we need a max here?
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> content/renderer/input/main_thread_event_queue.cc:13: bool
> isVSyncAlignedEvent(const std::unique_ptr<EventWithDispatchType>& event) {
> I'd name this something like eventCanBeRafAligned.
> 
> Or maybe something like "isContinousEvent", as opposed to discrete events?
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> content/renderer/input/main_thread_event_queue.cc:157: LOG(ERROR) <<
"Processed
> " << i << " events in RAF";
> Remember to remove this before landing (and make the for loop more normal).
> 
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> content/renderer/input/main_thread_event_queue.cc:168: bool isVsynced =
> isVSyncAlignedEvent(event);
> Inconsistent casing here.

PTAL; I documented some of the stuff in the short design doc

[dtapuska, 2016-08-11 21:10:10]

On 2016/08/11 21:07:23, dtapuska wrote:
> On 2016/07/20 21:43:09, tdresser wrote:
> > High level looks good. Let me know if I should do a more thorough review.
> > 
> > I noted a few nits as I went through.
> > 
> > I think we want to view this as BeginFrame aligned (or rAF aligned), not
VSync
> > aligned. For example in the BeginRetroFrame case, these could be pretty
> > different.
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
> > File content/common/input/web_input_event_queue.h (right):
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
> > content/common/input/web_input_event_queue.h:59: const std::unique_ptr<T>&
> Top()
> > {
> > Why Top instead of front?
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/inpu...
> > File content/renderer/input/input_event_filter.h (right):
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/inpu...
> > content/renderer/input/input_event_filter.h:73: void
> > ProcessVSyncAlignedInput(int routing_id) override;
> > I think we want to align with rAF more than we want to align with vsync.
> > Normally they're the same, but they won't always be (if I understand
> correctly).
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > File content/renderer/input/main_thread_event_queue.cc (right):
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > content/renderer/input/main_thread_event_queue.cc:12: const size_t
> > kMaxEventsPerVSyncEvent = 10;
> > Add a comment about this constant - why do we need a max here?
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > content/renderer/input/main_thread_event_queue.cc:13: bool
> > isVSyncAlignedEvent(const std::unique_ptr<EventWithDispatchType>& event) {
> > I'd name this something like eventCanBeRafAligned.
> > 
> > Or maybe something like "isContinousEvent", as opposed to discrete events?
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > content/renderer/input/main_thread_event_queue.cc:157: LOG(ERROR) <<
> "Processed
> > " << i << " events in RAF";
> > Remember to remove this before landing (and make the for loop more normal).
> > 
> >
>
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > content/renderer/input/main_thread_event_queue.cc:168: bool isVsynced =
> > isVSyncAlignedEvent(event);
> > Inconsistent casing here.
> 
> PTAL; I documented some of the stuff in the short design doc

I do need to write some tests for this. Although I'm not sure how I can make
them reliable.

[dtapuska, 2016-08-23 20:14:32]

Description was changed from

==========
A simple prototype of main thread raf aligned input.

Short Design Doc:

https://docs.google.com/document/d/1aM9AqyYuceRHOmsJZXFKcgi_D4jvFAymYvI36OEwI...

BUG=625693
==========

to

==========
Introduce the concept of RAF aligned input events to the MainThreadEventQueue.
Currently behind a feature flag and is disabled.

Short Design Doc:

https://docs.google.com/document/d/1aM9AqyYuceRHOmsJZXFKcgi_D4jvFAymYvI36OEwI...

BUG=625693
==========

[dtapuska, 2016-08-23 20:25:14]

On 2016/08/11 at 21:10:10, dtapuska wrote:
> On 2016/08/11 21:07:23, dtapuska wrote:
> > On 2016/07/20 21:43:09, tdresser wrote:
> > > High level looks good. Let me know if I should do a more thorough review.
> > > 
> > > I noted a few nits as I went through.
> > > 
> > > I think we want to view this as BeginFrame aligned (or rAF aligned), not
VSync
> > > aligned. For example in the BeginRetroFrame case, these could be pretty
> > > different.
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
> > > File content/common/input/web_input_event_queue.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/common/input/web_in...
> > > content/common/input/web_input_event_queue.h:59: const std::unique_ptr<T>&
> > Top()
> > > {
> > > Why Top instead of front?
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/inpu...
> > > File content/renderer/input/input_event_filter.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/inpu...
> > > content/renderer/input/input_event_filter.h:73: void
> > > ProcessVSyncAlignedInput(int routing_id) override;
> > > I think we want to align with rAF more than we want to align with vsync.
> > > Normally they're the same, but they won't always be (if I understand
> > correctly).
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > > File content/renderer/input/main_thread_event_queue.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > > content/renderer/input/main_thread_event_queue.cc:12: const size_t
> > > kMaxEventsPerVSyncEvent = 10;
> > > Add a comment about this constant - why do we need a max here?
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > > content/renderer/input/main_thread_event_queue.cc:13: bool
> > > isVSyncAlignedEvent(const std::unique_ptr<EventWithDispatchType>& event) {
> > > I'd name this something like eventCanBeRafAligned.
> > > 
> > > Or maybe something like "isContinousEvent", as opposed to discrete events?
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > > content/renderer/input/main_thread_event_queue.cc:157: LOG(ERROR) <<
> > "Processed
> > > " << i << " events in RAF";
> > > Remember to remove this before landing (and make the for loop more
normal).
> > > 
> > >
> >
https://codereview.chromium.org/2166703003/diff/1/content/renderer/input/main...
> > > content/renderer/input/main_thread_event_queue.cc:168: bool isVsynced =
> > > isVSyncAlignedEvent(event);
> > > Inconsistent casing here.
> > 
> > PTAL; I documented some of the stuff in the short design doc
> 
> I do need to write some tests for this. Although I'm not sure how I can make
them reliable.

PTAL. I am now done with this change % any issues you have.

[tdresser, 2016-08-24 13:43:00]

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter.cc (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter.cc:86: route_queues_[routing_id] = new
MainThreadEventQueue(
Should we move reading the flag into the MTEQ? It will be reading a few other
flags (fling intervention, unresponsive main thread intervention).

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter.h:39: // The user of this class
provides an instance of InputEventFilter::Handler,
Update comment.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter.h:112: // The handler_ only gets Run
on the thread corresponding to target_loop_.
Update comment.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter.h:114: InputHandlerManager*
input_handler_manager_;
Add a comment about lifetime of the input_handler_manager_.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:149:
event_recorder_.reset(new InputEventRecorder(filter_.get()));
Sounds like folks are preferring event_recorder_ =
MakeUnique<InputEventRecorder>(filter.get());

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_handler_manager_client.h (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:42: ui::LatencyInfo*
latency_info)> Handler;
Is this still used?

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

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:16: const size_t
kMaxEventsPerRafTask = 10;
I don't completely understand this constant.

Suppose we have a 10 second long main thread task. We can't collect all ~10 * 60
events and then dispatch them 10 at a time to the main thread.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:18: bool
isContiniousEvent(const std::unique_ptr<EventWithDispatchType>& event) {
Continuous

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:247: bool isContinious =
isContiniousEvent(event);
is_continuous

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

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:43: // |coalesced_event_ids_|
contains the unique touch event id to be acked. If
event id -> event ids

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:44: // the events are
TouchEvents the value will be 0. More importantly for
Is this comment correct? Should it be "If the events areN'T TouchEvents?"

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:49: base::TimeTicks
coalesced_timestamp_;
Maybe:
last_coalesced_timestamp_
or
most_recent_coalesced_timestamp_
?

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:153: bool
sent_main_frame_request_;
We should indicate which members should only be accessed through the
event_queue_lock_.

It might even be worth bundling them into a struct, though if it's only two
members, it's probably be fine.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:41:
"Event.MainThreadEventQueue.CoalescedCount";
Missing space

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:54:
needs_main_frame_(false) {}
This indentation looks off.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:272:
EXPECT_EQ(GetParam(), !main_task_runner_->HasPendingTask());
Save the parameter into a well named variable.

I think you can do this in the constructor, and then have a function that
returns the parameter value, which is probably the cleanest way to do it.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:304: // Don't run the
test for when we aren't supporting raf aligned.
Don't run the test when we aren't supporting rAF aligned input.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:322: // Simulate
enqueing a normal event, followed by "continious" events and then
continuous

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:323: // a normal
event. The "continious" events should not wait for RAF.
Is the above comment correct? Isn't it the discrete events that should not wait
for rAF?

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:323: // a normal
event. The "continious" events should not wait for RAF.
nittiest nit: We normally write rAF, as it abbreviates requestAnimationFrame.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:326:
HandleEvent(kWheelEvents[0], INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING);
If we aren't iterating through the events, I'd rather we give them logical
names, instead of shoving them in an array.

kMouseDownEvent, kMouseMoveEvent, kMouseUpEvent.

The wheel events are fine.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:561:
INSTANTIATE_TEST_CASE_P(MainThreadEventQueueTests,
Add a comment indicating what the bool means.

[dtapuska, 2016-08-24 16:40:35]

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

[commit-bot: I haz the power, 2016-08-24 16:41:16]

Dry run: 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, 2016-08-24 16:51:09]

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

[commit-bot: I haz the power, 2016-08-24 16:51:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dtapuska, 2016-08-24 17:10:07]

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter.cc (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter.cc:86: route_queues_[routing_id] = new
MainThreadEventQueue(
On 2016/08/24 at 13:42:59, tdresser wrote:
> Should we move reading the flag into the MTEQ? It will be reading a few other
flags (fling intervention, unresponsive main thread intervention).

I guess. I did it this way as it made TEST_P easier. But I do see that others
are adding feature checks inside the class itself.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter.h:112: // The handler_ only gets Run
on the thread corresponding to target_loop_.
On 2016/08/24 at 13:42:59, tdresser wrote:
> Update comment.

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter.h:114: InputHandlerManager*
input_handler_manager_;
On 2016/08/24 at 13:42:59, tdresser wrote:
> Add a comment about lifetime of the input_handler_manager_.

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_event_filter_unittest.cc (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_event_filter_unittest.cc:149:
event_recorder_.reset(new InputEventRecorder(filter_.get()));
On 2016/08/24 at 13:42:59, tdresser wrote:
> Sounds like folks are preferring event_recorder_ =
MakeUnique<InputEventRecorder>(filter.get());

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/input_handler_manager_client.h (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/input_handler_manager_client.h:42: ui::LatencyInfo*
latency_info)> Handler;
On 2016/08/24 at 13:42:59, tdresser wrote:
> Is this still used?

removed.

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

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:16: const size_t
kMaxEventsPerRafTask = 10;
On 2016/08/24 at 13:42:59, tdresser wrote:
> I don't completely understand this constant.
> 
> Suppose we have a 10 second long main thread task. We can't collect all ~10 *
60 events and then dispatch them 10 at a time to the main thread.

So you want to send them all to the main thread at once? There is the potential
that this is an infinite loop; but we could take a snapshot of the queue size at
the beginning and process that many. Let's talk about it.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:18: bool
isContiniousEvent(const std::unique_ptr<EventWithDispatchType>& event) {
On 2016/08/24 at 13:42:59, tdresser wrote:
> Continuous

done; face palm.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:247: bool isContinious =
isContiniousEvent(event);
On 2016/08/24 at 13:42:59, tdresser wrote:
> is_continuous

done

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

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:43: // |coalesced_event_ids_|
contains the unique touch event id to be acked. If
On 2016/08/24 at 13:43:00, tdresser wrote:
> event id -> event ids

done.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:44: // the events are
TouchEvents the value will be 0. More importantly for
On 2016/08/24 at 13:43:00, tdresser wrote:
> Is this comment correct? Should it be "If the events areN'T TouchEvents?"

done.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:49: base::TimeTicks
coalesced_timestamp_;
On 2016/08/24 at 13:43:00, tdresser wrote:
> Maybe:
> last_coalesced_timestamp_
> or
> most_recent_coalesced_timestamp_
> ?

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:153: bool
sent_main_frame_request_;
On 2016/08/24 at 13:42:59, tdresser wrote:
> We should indicate which members should only be accessed through the
event_queue_lock_.
> 
> It might even be worth bundling them into a struct, though if it's only two
members, it's probably be fine.

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:41:
"Event.MainThreadEventQueue.CoalescedCount";
On 2016/08/24 at 13:43:00, tdresser wrote:
> Missing space

you mean a LF? done.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:54:
needs_main_frame_(false) {}
On 2016/08/24 at 13:43:00, tdresser wrote:
> This indentation looks off.

how so; this is run through git cl format.

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:272:
EXPECT_EQ(GetParam(), !main_task_runner_->HasPendingTask());
On 2016/08/24 at 13:43:00, tdresser wrote:
> Save the parameter into a well named variable.
> 
> I think you can do this in the constructor, and then have a function that
returns the parameter value, which is probably the cleanest way to do it.

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:304: // Don't run the
test for when we aren't supporting raf aligned.
On 2016/08/24 at 13:43:00, tdresser wrote:
> Don't run the test when we aren't supporting rAF aligned input.

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:322: // Simulate
enqueing a normal event, followed by "continious" events and then
On 2016/08/24 at 13:43:00, tdresser wrote:
> continuous

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:323: // a normal
event. The "continious" events should not wait for RAF.
On 2016/08/24 at 13:43:00, tdresser wrote:
> nittiest nit: We normally write rAF, as it abbreviates requestAnimationFrame.

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:326:
HandleEvent(kWheelEvents[0], INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING);
On 2016/08/24 at 13:43:00, tdresser wrote:
> If we aren't iterating through the events, I'd rather we give them logical
names, instead of shoving them in an array.
> 
> kMouseDownEvent, kMouseMoveEvent, kMouseUpEvent.
> 
> The wheel events are fine.

done

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:561:
INSTANTIATE_TEST_CASE_P(MainThreadEventQueueTests,
On 2016/08/24 at 13:43:00, tdresser wrote:
> Add a comment indicating what the bool means.

done

[dtapuska, 2016-08-24 17:37:19]

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

[commit-bot: I haz the power, 2016-08-24 17:38:00]

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

[tdresser, 2016-08-24 17:45:16]

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2166703003/diff/40001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:54:
needs_main_frame_(false) {}
On 2016/08/24 17:10:07, dtapuska wrote:
> On 2016/08/24 at 13:43:00, tdresser wrote:
> > This indentation looks off.
> 
> how so; this is run through git cl format.

Nevermind.

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/input_event_filter.h:41: // WebInputEvents on the target
thread.
Refer to the method called.

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/input_event_filter.h:113: // The |input_handler_manager_|
should outlive the life of this class and
outlive the life of -> outlive

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

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:37:
last_coalesced_timestamp_(creation_timestamp_) {}
I was thinking these timestamps would be pulled from the events, not from the
clock.

creation_timestamp_ would come from event, last_coalsced_timestamp_ would come
from |other| in CoalesceWith.

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:143: (base::TimeTicks::Now() -
in_flight_event_->creationTimestamp())
Factor out call to now, to avoid repeated calls.

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:145: 1, 10000000, 100);
We should only use 50 buckets for all of these.

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:210: // We should only have
blocking touch events that need coalescing.
Aren't some of these not touch events?

[commit-bot: I haz the power, 2016-08-24 21:25:14]

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

[commit-bot: I haz the power, 2016-08-24 21:25:15]

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

[dtapuska, 2016-08-25 14:07:23]

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/input_event_filter.h:41: // WebInputEvents on the target
thread.
On 2016/08/24 at 17:45:16, tdresser wrote:
> Refer to the method called.

done

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/input_event_filter.h:113: // The |input_handler_manager_|
should outlive the life of this class and
On 2016/08/24 at 17:45:16, tdresser wrote:
> outlive the life of -> outlive

done

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

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:37:
last_coalesced_timestamp_(creation_timestamp_) {}
On 2016/08/24 17:45:16, tdresser wrote:
> I was thinking these timestamps would be pulled from the events, not from the
> clock.
> 
> creation_timestamp_ would come from event, last_coalsced_timestamp_ would come
> from |other| in CoalesceWith.

I think using Now represents how long the events are in the queue explicitly.

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:143: (base::TimeTicks::Now() -
in_flight_event_->creationTimestamp())
On 2016/08/24 at 17:45:16, tdresser wrote:
> Factor out call to now, to avoid repeated calls.

done

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:145: 1, 10000000, 100);
On 2016/08/24 at 17:45:16, tdresser wrote:
> We should only use 50 buckets for all of these.

done

https://codereview.chromium.org/2166703003/diff/50001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:210: // We should only have
blocking touch events that need coalescing.
On 2016/08/24 at 17:45:16, tdresser wrote:
> Aren't some of these not touch events?

Yes; I've removed the comment which was copied due to re-arranged code which is
no longer true.

[tdresser, 2016-08-25 14:38:41]

Some of the things indicated "done" aren't done.

Did you forget to upload?

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
content/renderer/input/input_event_filter.h:113: // The |input_handler_manager_|
should outlive the life of this class and
"outlive the life of" -> outlive

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

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:30: }
Missing newline.

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

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:163: // Lock used to serailize
|shared_state_|.
serialize

[dtapuska, 2016-08-25 15:01:23]

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

[dtapuska, 2016-08-25 15:01:36]

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
File content/renderer/input/input_event_filter.h (right):

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
content/renderer/input/input_event_filter.h:113: // The |input_handler_manager_|
should outlive the life of this class and
On 2016/08/25 14:38:41, tdresser wrote:
> "outlive the life of" -> outlive

Done.

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

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:30: }
On 2016/08/25 14:38:41, tdresser wrote:
> Missing newline.

Done.

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

https://codereview.chromium.org/2166703003/diff/70001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.h:163: // Lock used to serailize
|shared_state_|.
On 2016/08/25 14:38:41, tdresser wrote:
> serialize

Done.

[commit-bot: I haz the power, 2016-08-25 15:02:07]

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

[tdresser, 2016-08-25 15:16:43]

LGTM, except for question about relocking per event.

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

https://codereview.chromium.org/2166703003/diff/90001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:157:
in_flight_event_->coalescedEventIds().size());
A 2 second jank will often leave us with > 100 coaesced events.

I'd be tempted to use a log scale, 50 buckets, with a max of 1000.

https://codereview.chromium.org/2166703003/diff/90001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:233: base::AutoLock
lock(shared_state_lock_);
Is it worth dropping and reacquiring this lock every iteration?

Feels like it adds more complexity than it's worth (risk of the compositor
feeding us events too quickly).

We could get the full list of events to send, release the lock, and then
dispatch them all.

[dtapuska, 2016-08-25 15:30:06]

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

https://codereview.chromium.org/2166703003/diff/90001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:157:
in_flight_event_->coalescedEventIds().size());
On 2016/08/25 at 15:16:43, tdresser wrote:
> A 2 second jank will often leave us with > 100 coaesced events.
> 
> I'd be tempted to use a log scale, 50 buckets, with a max of 1000.

bumped to 1000.

https://codereview.chromium.org/2166703003/diff/90001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:233: base::AutoLock
lock(shared_state_lock_);
On 2016/08/25 at 15:16:43, tdresser wrote:
> Is it worth dropping and reacquiring this lock every iteration?
> 
> Feels like it adds more complexity than it's worth (risk of the compositor
feeding us events too quickly).
> 
> We could get the full list of events to send, release the lock, and then
dispatch them all.

Its a toss up. The expected size is always 1. Do we always want to ensure there
is only one input per frame essentially or not. With Rick's event timing example
it is possible that you can get two if the timing is right in that an event is
removed from the queue; you invoke the main thread callback while another one
was just put into the queue. This design we will run it right away instead of
waiting around for another main frame callback.

[tdresser, 2016-08-25 15:38:27]

Yeah, this is fine.

[dtapuska, 2016-08-25 15:43:49]

dtapuska@chromium.org changed reviewers:
+ isherman@chromium.org, pfeldman@chromium.org

[dtapuska, 2016-08-25 15:43:52]

isherman@ please review tools/metrics
pfeldman@ please review content/* minus the input which tdresser@ has reviewed.

[Ilya Sherman, 2016-08-25 19:34:16]

metrics lgtm % nits:

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

https://codereview.chromium.org/2166703003/diff/110001/content/renderer/input...
content/renderer/input/main_thread_event_queue.cc:148: (now -
in_flight_event_->creationTimestamp()).ToInternalValue(), 1,
Please call InMicroseconds() rather than ToInternalValue().  The internal value
is an implementation detail that is subject to change.

https://codereview.chromium.org/2166703003/diff/110001/content/renderer/input...
content/renderer/input/main_thread_event_queue.cc:149: 10000000, 50);
nit: This constant is very hard to parse.  I'd write it as 10 * 1000 * 1000, and
maybe even store it in a constant named kTenSeconds.

https://codereview.chromium.org/2166703003/diff/110001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2166703003/diff/110001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:13450: +    inside the main thread event
queue.
Please document precisely when this is recorded.

[dtapuska, 2016-08-25 19:57:29]

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

https://codereview.chromium.org/2166703003/diff/110001/content/renderer/input...
content/renderer/input/main_thread_event_queue.cc:148: (now -
in_flight_event_->creationTimestamp()).ToInternalValue(), 1,
On 2016/08/25 at 19:34:16, Ilya Sherman wrote:
> Please call InMicroseconds() rather than ToInternalValue().  The internal
value is an implementation detail that is subject to change.

done

https://codereview.chromium.org/2166703003/diff/110001/content/renderer/input...
content/renderer/input/main_thread_event_queue.cc:149: 10000000, 50);
On 2016/08/25 at 19:34:16, Ilya Sherman wrote:
> nit: This constant is very hard to parse.  I'd write it as 10 * 1000 * 1000,
and maybe even store it in a constant named kTenSeconds.

done; copy and paste from other places :-(

https://codereview.chromium.org/2166703003/diff/110001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2166703003/diff/110001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:13450: +    inside the main thread event
queue.
On 2016/08/25 at 19:34:16, Ilya Sherman wrote:
> Please document precisely when this is recorded.

done.

[dtapuska, 2016-08-29 14:32:51]

On 2016/08/25 at 19:57:29, dtapuska wrote:
>
https://codereview.chromium.org/2166703003/diff/110001/content/renderer/input...
> File content/renderer/input/main_thread_event_queue.cc (right):
> 
>
https://codereview.chromium.org/2166703003/diff/110001/content/renderer/input...
> content/renderer/input/main_thread_event_queue.cc:148: (now -
in_flight_event_->creationTimestamp()).ToInternalValue(), 1,
> On 2016/08/25 at 19:34:16, Ilya Sherman wrote:
> > Please call InMicroseconds() rather than ToInternalValue().  The internal
value is an implementation detail that is subject to change.
> 
> done
> 
>
https://codereview.chromium.org/2166703003/diff/110001/content/renderer/input...
> content/renderer/input/main_thread_event_queue.cc:149: 10000000, 50);
> On 2016/08/25 at 19:34:16, Ilya Sherman wrote:
> > nit: This constant is very hard to parse.  I'd write it as 10 * 1000 * 1000,
and maybe even store it in a constant named kTenSeconds.
> 
> done; copy and paste from other places :-(
> 
>
https://codereview.chromium.org/2166703003/diff/110001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2166703003/diff/110001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:13450: +    inside the main thread
event queue.
> On 2016/08/25 at 19:34:16, Ilya Sherman wrote:
> > Please document precisely when this is recorded.
> 
> done.

pfeldman@ ping

[pfeldman, 2016-08-29 17:36:52]

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
content/renderer/render_view_impl.cc:1667: void RenderViewImpl::NeedsMainFrame()
{
Ah, so it mutates the compositor state - was not clear from the name. Please
provide a more descriptive name. In fact, it does not look like it belongs to
the render_view_impl altogether.

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
content/renderer/render_view_impl.h:242: void NeedsMainFrame();
MainFrame sounds ambiguous in the context of render_view_impl. Is that
render_frame or a rendering frame?

[dtapuska, 2016-08-29 19:32:36]

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
content/renderer/render_view_impl.cc:1667: void RenderViewImpl::NeedsMainFrame()
{
On 2016/08/29 at 17:36:52, pfeldman wrote:
> Ah, so it mutates the compositor state - was not clear from the name. Please
provide a more descriptive name. In fact, it does not look like it belongs to
the render_view_impl altogether.

I've moved it to render_widget and moved it to be close to the BeginMainFrame
impl.

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/2166703003/diff/130001/content/renderer/rende...
content/renderer/render_view_impl.h:242: void NeedsMainFrame();
On 2016/08/29 at 17:36:52, pfeldman wrote:
> MainFrame sounds ambiguous in the context of render_view_impl. Is that
render_frame or a rendering frame?

see other comment

[pfeldman, 2016-08-29 20:52:13]

lgtm

[dtapuska, 2016-08-29 20:52:41]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-08-29 20:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2166703003/#ps150001
(title: "Rename NeedsMainFrame to SetNeedsMainFrame and move it to the
render_widget")

[commit-bot: I haz the power, 2016-08-29 21:11:39]

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, 2016-08-30 04:34:18]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-08-30 04:37:07]

Description was changed from

==========
Introduce the concept of RAF aligned input events to the MainThreadEventQueue.
Currently behind a feature flag and is disabled.

Short Design Doc:

https://docs.google.com/document/d/1aM9AqyYuceRHOmsJZXFKcgi_D4jvFAymYvI36OEwI...

BUG=625693
==========

to

==========
Introduce the concept of RAF aligned input events to the MainThreadEventQueue.
Currently behind a feature flag and is disabled.

Short Design Doc:

https://docs.google.com/document/d/1aM9AqyYuceRHOmsJZXFKcgi_D4jvFAymYvI36OEwI...

BUG=625693

Committed: https://crrev.com/3d5624d3a3aa88e0fafe5bd09b5725b1b3f25d67
Cr-Commit-Position: refs/heads/master@{#415099}
==========

[commit-bot: I haz the power, 2016-08-30 04:37:11]

Patchset 9 (id:??) landed as
https://crrev.com/3d5624d3a3aa88e0fafe5bd09b5725b1b3f25d67
Cr-Commit-Position: refs/heads/master@{#415099}