Add latency info to input events sent to RenderWidget.

Add latency info to input events sent to RenderWidget.

This keeps track of the latency info for input events, and sends that down into the RenderWidget and into the compositor. It also collects the latency info when swap happens with gtk and returns it the the RenderWidgetHostImpl.

BUG=155367
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199340

[piman, 2013-04-22 20:46:38]

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/gesture_event_filter.h (right):

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
content/browser/renderer_host/gesture_event_filter.h:23: class
GestureEventLatency : public WebKit::WebGestureEvent {
nit: I found this name confusing in other files. How about
GestureEventWithLatencyInfo?

Does it have to inherit from WebGestureEvent, or could it be a struct with a
WebGestureEvent and a LatencyInfo? The latter would avoid accidental
upcasts/slicing.

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
content/browser/renderer_host/gesture_event_filter.h:28: const cc::LatencyInfo&
l) :
nit: ':' on next line, at +4

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1178: return
GetRoutingID() | ((int64)GetProcess()->GetID() << 32);
nit: no c-style casts.

qq: does anything bad happens when process IDs wrap and get reused?

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1181: cc::LatencyInfo
RenderWidgetHostImpl::NewInputLatencyInfo() {
It's ok to do it by steps, but I want to make sure that we have the long term
vision: there can be sources of input latency in the UI layer (e.g. its own
coalescing), so I'm hoping the latency information will be passed from the UI
layer to the RWHI

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.h:471: class
FrameSwappedObserver {
There doesn't seem to be any implementation of this in this patch - is this
"FrameSwapped" infrastructure for a later patch? Can you remove it from this
one, so that we can review that part later, in context?

https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.h:781: typedef
std::deque<std::pair<WebKit::WebMouseWheelEvent, cc::LatencyInfo> >
nit: rather than a std::pair, an explicit struct is a lot more readable at the
call sites ("event", "latency_info" as fields instead of "first" and "second")

[sadrul, 2013-04-22 20:56:03]

Would it be possible to include the latency-info into the WebInputEvent itself
(perhaps a WebLatencyInfo)? I think that would simplify some of the code, and
would fit in nicely with the UI layer providing the necessary info when it
creates the Web-events.

[piman, 2013-04-22 21:01:48]

On 2013/04/22 20:56:03, sadrul wrote:
> Would it be possible to include the latency-info into the WebInputEvent itself
> (perhaps a WebLatencyInfo)? I think that would simplify some of the code, and
> would fit in nicely with the UI layer providing the necessary info when it
> creates the Web-events.

That's a good idea, and I think it makes a lot of sense.

There is currently one problem in that LatencyInfo is in cc, which depends on
WebKit. Having WebInputEvent depend on cc/ would cause a circular dependency.
However, in the grand scheme of things, it's conceivable that LatencyInfo should
move up somewhere since it's used for things beyond cc/

[jbauman, 2013-04-22 21:35:12]

On 2013/04/22 20:56:03, sadrul wrote:
> Would it be possible to include the latency-info into the WebInputEvent itself
> (perhaps a WebLatencyInfo)? I think that would simplify some of the code, and
> would fit in nicely with the UI layer providing the necessary info when it
> creates the Web-events.

One issue is that WebInputEvent and descendants all have to be fixed-size,
bitwise-copyable PODs, so any structure we stored with them would have to be
much more limited than the normal cc::LatencyInfo, which can hold information
for a very large number of components.

[jbauman, 2013-04-22 21:43:22]

On 2013/04/22 20:46:38, piman wrote:
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/gesture_event_filter.h (right):
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/gesture_event_filter.h:23: class
> GestureEventLatency : public WebKit::WebGestureEvent {
> nit: I found this name confusing in other files. How about
> GestureEventWithLatencyInfo?

We start to run up against the 80-col limit with that name, but it could work.
> 
> Does it have to inherit from WebGestureEvent, or could it be a struct with a
> WebGestureEvent and a LatencyInfo? The latter would avoid accidental
> upcasts/slicing.

Having it inherit from WebGestureEvent makes a lot of code that consumes the
events somewhat nicer, but I could change that if you want.
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/gesture_event_filter.h:28: const
cc::LatencyInfo&
> l) :
> nit: ':' on next line, at +4
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.cc:1178: return
> GetRoutingID() | ((int64)GetProcess()->GetID() << 32);
> nit: no c-style casts.
> 
> qq: does anything bad happens when process IDs wrap and get reused?
> 
Certainly what happens in this code is no worse than what happens in the rest of
the code (where duplicate routing IDs cause rphi creation to probably fail in
weird ways).
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.cc:1181: cc::LatencyInfo
> RenderWidgetHostImpl::NewInputLatencyInfo() {
> It's ok to do it by steps, but I want to make sure that we have the long term
> vision: there can be sources of input latency in the UI layer (e.g. its own
> coalescing), so I'm hoping the latency information will be passed from the UI
> layer to the RWHI
> 
Yeah, I haven't planned this out yet completely, but we should be able to create
the LatencyInfo in the platform code and pass it along as well at some point.
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_impl.h (right):
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.h:471: class
> FrameSwappedObserver {
> There doesn't seem to be any implementation of this in this patch - is this
> "FrameSwapped" infrastructure for a later patch? Can you remove it from this
> one, so that we can review that part later, in context?
> 
Ok, sure.
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.h:781: typedef
> std::deque<std::pair<WebKit::WebMouseWheelEvent, cc::LatencyInfo> >
> nit: rather than a std::pair, an explicit struct is a lot more readable at the
> call sites ("event", "latency_info" as fields instead of "first" and "second")

[piman, 2013-04-22 21:56:23]

On Mon, Apr 22, 2013 at 2:43 PM, <jbauman@chromium.org> wrote:

> On 2013/04/22 20:46:38, piman wrote:
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**gesture_event_filter.h<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/gesture_event_filter.h>
>
>> File content/browser/renderer_host/**gesture_event_filter.h (right):
>>
>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**gesture_event_filter.h#**newcode23<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/gesture_event_filter.h#newcode23>
>
>> content/browser/renderer_host/**gesture_event_filter.h:23: class
>> GestureEventLatency : public WebKit::WebGestureEvent {
>> nit: I found this name confusing in other files. How about
>> GestureEventWithLatencyInfo?
>>
>
> We start to run up against the 80-col limit with that name, but it could
> work.
>
>
>  Does it have to inherit from WebGestureEvent, or could it be a struct
>> with a
>> WebGestureEvent and a LatencyInfo? The latter would avoid accidental
>> upcasts/slicing.
>>
>
> Having it inherit from WebGestureEvent makes a lot of code that consumes
> the
> events somewhat nicer, but I could change that if you want.


How so? This would only need a field dereference instead of an implicit
cast, right?

>
>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**gesture_event_filter.h#**newcode28<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/gesture_event_filter.h#newcode28>
>
>> content/browser/renderer_host/**gesture_event_filter.h:28: const
>>
> cc::LatencyInfo&
>
>> l) :
>> nit: ':' on next line, at +4
>>
>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**render_widget_host_impl.cc<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/render_widget_host_impl.cc>
>
>> File content/browser/renderer_host/**render_widget_host_impl.cc (right):
>>
>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**render_widget_host_impl.cc#**newcode1178<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/render_widget_host_impl.cc#newcode1178>
>
>> content/browser/renderer_host/**render_widget_host_impl.cc:**1178: return
>> GetRoutingID() | ((int64)GetProcess()->GetID() << 32);
>> nit: no c-style casts.
>>
>
>  qq: does anything bad happens when process IDs wrap and get reused?
>>
>
> Certainly what happens in this code is no worse than what happens in the
> rest of
> the code (where duplicate routing IDs cause rphi creation to probably fail
> in
> weird ways).


Oh, NVM, this is not the pid.


>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**render_widget_host_impl.cc#**newcode1181<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/render_widget_host_impl.cc#newcode1181>
>
>> content/browser/renderer_host/**render_widget_host_impl.cc:**1181:
>> cc::LatencyInfo
>> RenderWidgetHostImpl::**NewInputLatencyInfo() {
>> It's ok to do it by steps, but I want to make sure that we have the long
>> term
>> vision: there can be sources of input latency in the UI layer (e.g. its
>> own
>> coalescing), so I'm hoping the latency information will be passed from
>> the UI
>> layer to the RWHI
>>
>
> Yeah, I haven't planned this out yet completely, but we should be able to
> create
> the LatencyInfo in the platform code and pass it along as well at some
> point.
>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**render_widget_host_impl.h<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/render_widget_host_impl.h>
>
>> File content/browser/renderer_host/**render_widget_host_impl.h (right):
>>
>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**render_widget_host_impl.h#**newcode471<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/render_widget_host_impl.h#newcode471>
>
>> content/browser/renderer_host/**render_widget_host_impl.h:471: class
>> FrameSwappedObserver {
>> There doesn't seem to be any implementation of this in this patch - is
>> this
>> "FrameSwapped" infrastructure for a later patch? Can you remove it from
>> this
>> one, so that we can review that part later, in context?
>>
>
> Ok, sure.
>
>
> https://codereview.chromium.**org/13931009/diff/11001/**
>
content/browser/renderer_host/**render_widget_host_impl.h#**newcode781<https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_host/render_widget_host_impl.h#newcode781>
>
>> content/browser/renderer_host/**render_widget_host_impl.h:781: typedef
>> std::deque<std::pair<WebKit::**WebMouseWheelEvent, cc::LatencyInfo> >
>> nit: rather than a std::pair, an explicit struct is a lot more readable
>> at the
>> call sites ("event", "latency_info" as fields instead of "first" and
>> "second")
>>
>
>
>
>
https://codereview.chromium.**org/13931009/<https://codereview.chromium.org/1...
>

[jbauman, 2013-04-23 19:58:04]

Ok, I made a generic EventWithLatencyInfo that's templated on the specific input
event, so it can be used in a bunch more places.

On 2013/04/22 20:46:38, piman wrote:
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/gesture_event_filter.h (right):
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/gesture_event_filter.h:23: class
> GestureEventLatency : public WebKit::WebGestureEvent {
> nit: I found this name confusing in other files. How about
> GestureEventWithLatencyInfo?
> 
> Does it have to inherit from WebGestureEvent, or could it be a struct with a
> WebGestureEvent and a LatencyInfo? The latter would avoid accidental
> upcasts/slicing.
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/gesture_event_filter.h:28: const
cc::LatencyInfo&
> l) :
> nit: ':' on next line, at +4
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.cc:1178: return
> GetRoutingID() | ((int64)GetProcess()->GetID() << 32);
> nit: no c-style casts.
> 
> qq: does anything bad happens when process IDs wrap and get reused?
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.cc:1181: cc::LatencyInfo
> RenderWidgetHostImpl::NewInputLatencyInfo() {
> It's ok to do it by steps, but I want to make sure that we have the long term
> vision: there can be sources of input latency in the UI layer (e.g. its own
> coalescing), so I'm hoping the latency information will be passed from the UI
> layer to the RWHI
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_impl.h (right):
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.h:471: class
> FrameSwappedObserver {
> There doesn't seem to be any implementation of this in this patch - is this
> "FrameSwapped" infrastructure for a later patch? Can you remove it from this
> one, so that we can review that part later, in context?
> 
>
https://codereview.chromium.org/13931009/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_impl.h:781: typedef
> std::deque<std::pair<WebKit::WebMouseWheelEvent, cc::LatencyInfo> >
> nit: rather than a std::pair, an explicit struct is a lot more readable at the
> call sites ("event", "latency_info" as fields instead of "first" and "second")

[piman, 2013-04-25 04:20:07]

LGTM, nice!

[jbauman, 2013-04-25 22:29:20]

Also adding cevans@ to hopefully rubber-stamp the change to view_messages.h

[jbauman, 2013-04-30 01:47:39]

On 2013/04/25 22:29:20, jbauman wrote:
> Also adding cevans@ to hopefully rubber-stamp the change to view_messages.h

cevans@, ping?

[jbauman, 2013-05-08 23:04:35]

On 2013/04/30 01:47:39, jbauman wrote:
> On 2013/04/25 22:29:20, jbauman wrote:
> > Also adding cevans@ to hopefully rubber-stamp the change to view_messages.h
> 
> cevans@, ping?

And ping again. Or should I get someone else to review this?

[cevans, 2013-05-09 02:21:01]

On 2013/05/08 23:04:35, jbauman wrote:
> On 2013/04/30 01:47:39, jbauman wrote:
> > On 2013/04/25 22:29:20, jbauman wrote:
> > > Also adding cevans@ to hopefully rubber-stamp the change to
view_messages.h
> > 
> > cevans@, ping?
> 
> And ping again. Or should I get someone else to review this?

I've been on a leave, hence the lack of review.
I'm happy to still help but I don't see any change to view_messages.h

[jbauman, 2013-05-09 06:26:34]

On 2013/05/09 02:21:01, cevans wrote:
> On 2013/05/08 23:04:35, jbauman wrote:
> > On 2013/04/30 01:47:39, jbauman wrote:
> > > On 2013/04/25 22:29:20, jbauman wrote:
> > > > Also adding cevans@ to hopefully rubber-stamp the change to
> view_messages.h
> > > 
> > > cevans@, ping?
> > 
> > And ping again. Or should I get someone else to review this?
> 
> I've been on a leave, hence the lack of review.
> I'm happy to still help but I don't see any change to view_messages.h

Oh, sorry to bother you; I didn't see anything on your calendar. The change
moved to input_messages.h due to a refactoring.

[Chris Evans, 2013-05-09 17:46:41]

On 2013/05/09 06:26:34, jbauman wrote:
> On 2013/05/09 02:21:01, cevans wrote:
> > On 2013/05/08 23:04:35, jbauman wrote:
> > > On 2013/04/30 01:47:39, jbauman wrote:
> > > > On 2013/04/25 22:29:20, jbauman wrote:
> > > > > Also adding cevans@ to hopefully rubber-stamp the change to
> > view_messages.h
> > > > 
> > > > cevans@, ping?
> > > 
> > > And ping again. Or should I get someone else to review this?
> > 
> > I've been on a leave, hence the lack of review.
> > I'm happy to still help but I don't see any change to view_messages.h
> 
> Oh, sorry to bother you; I didn't see anything on your calendar. The change
> moved to input_messages.h due to a refactoring.

LGTM

[commit-bot: I haz the power, 2013-05-09 22:50:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/13931009/64001

[commit-bot: I haz the power, 2013-05-10 02:41:56]

Change committed as 199340