Add UMA/Telemetry stats for end-to-end scroll latency

Add UMA/Telemetry stats for end-to-end scroll latency

This CL adds the UMA/Telemetry metric for measuring the end-to-end
scroll latency from when the original touch events are created to
when the final frame is swapped.

BUG=246034
TEST=1. Run telemetry smoothness test and make sure average_scroll_update_latency
	is in the result.
     2. Check that chrome://histograms/Event.Latency.TouchToScrollUpdateSwap
        exist after scrolling some webpages.

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

[Rick Byers, 2013-07-10 01:36:30]

Looks reasonable to me with one suggestion.

https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:1154:
latency_info.MergeWith(ui_latency);
Why do we need two different LatencyInfo objects here?  Conceptually I would
have expected that we'd just call ui_latency.AddLatencyNumber and not need
latency_info at all...

https://codereview.chromium.org/18937002/diff/1/ui/base/latency_info.h
File ui/base/latency_info.h (right):

https://codereview.chromium.org/18937002/diff/1/ui/base/latency_info.h#newcode27
ui/base/latency_info.h:27: // It is now only set for GestureScrollXXX events.
Please just rename this to 'GestureScroll' instead of 'Injected' and update the
comments accordingly.  I think the code that looks for this will make more sense
then.

[Yufeng Shen (Slow to review), 2013-07-10 15:12:04]

https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:1154:
latency_info.MergeWith(ui_latency);
On 2013/07/10 01:36:30, Rick Byers wrote:
> Why do we need two different LatencyInfo objects here?  Conceptually I would
> have expected that we'd just call ui_latency.AddLatencyNumber and not need
> latency_info at all...

right. it is just that it happens to be coded this way. Originally without
ui_latency, there is only one latency_info = NewInputLatencyInfo() is created
here and passed on. Then there comes the ui_latency, and we need to merge what
is in NewInputLatencyInfo() into ui_latency. We can directly add what's added in
NewInputLatencyInfo() into ui_latency (it just adds RWH_COMPONENT), but since we
are using NewInputLatencyInfo() everywhere, I feel it is better just call
NewInputLatencyInfo() and merge the result with ui_latency.

https://codereview.chromium.org/18937002/diff/1/ui/base/latency_info.h
File ui/base/latency_info.h (right):

https://codereview.chromium.org/18937002/diff/1/ui/base/latency_info.h#newcode27
ui/base/latency_info.h:27: // It is now only set for GestureScrollXXX events.
On 2013/07/10 01:36:30, Rick Byers wrote:
> Please just rename this to 'GestureScroll' instead of 'Injected' and update
the
> comments accordingly.  I think the code that looks for this will make more
sense
> then.

Done.

[Rick Byers, 2013-07-10 15:31:35]

https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:1154:
latency_info.MergeWith(ui_latency);
On 2013/07/10 15:12:04, Yufeng Shen wrote:
> On 2013/07/10 01:36:30, Rick Byers wrote:
> > Why do we need two different LatencyInfo objects here?  Conceptually I would
> > have expected that we'd just call ui_latency.AddLatencyNumber and not need
> > latency_info at all...
> 
> right. it is just that it happens to be coded this way. Originally without
> ui_latency, there is only one latency_info = NewInputLatencyInfo() is created
> here and passed on. Then there comes the ui_latency, and we need to merge what
> is in NewInputLatencyInfo() into ui_latency. We can directly add what's added
in
> NewInputLatencyInfo() into ui_latency (it just adds RWH_COMPONENT), but since
we
> are using NewInputLatencyInfo() everywhere, I feel it is better just call
> NewInputLatencyInfo() and merge the result with ui_latency. 

Ok, I see.  But now that you're only conditionally merging ui_latency into
latency_info, doesn't that mean in the non-gesture cases the data in ui_latency
is getting dropped on the floor?  So I think you at least want to merge it in
outside the if block.

It would be clearer to me if we refactored NewInputLatencyInfo to a function
that adds the RWH_COMPONENT to an existing latency info so we're always either
creating or extending a LatencyInfo.

https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:1140: //
INPUT_EVENT_LATENCY_RWH_COMPONENT right here.
We should probably work to unify Android and Aura here.  Please file a bug to
say Android should be propagating the latency_info for a touch event into the
gesture event.  If you'd like to try putting up a CL for that, that's great,
otherwise we can ask klobag@/aelias@ if someone on the Clank team wants to do
it.

These numbers will be most useful when they have a consistent meaning across all
platforms...

[Yufeng Shen (Slow to review), 2013-07-10 16:34:07]

On 2013/07/10 15:31:35, Rick Byers wrote:
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_widget_host_impl.cc:1154:
> latency_info.MergeWith(ui_latency);
> On 2013/07/10 15:12:04, Yufeng Shen wrote:
> > On 2013/07/10 01:36:30, Rick Byers wrote:
> > > Why do we need two different LatencyInfo objects here?  Conceptually I
would
> > > have expected that we'd just call ui_latency.AddLatencyNumber and not need
> > > latency_info at all...
> > 
> > right. it is just that it happens to be coded this way. Originally without
> > ui_latency, there is only one latency_info = NewInputLatencyInfo() is
created
> > here and passed on. Then there comes the ui_latency, and we need to merge
what
> > is in NewInputLatencyInfo() into ui_latency. We can directly add what's
added
> in
> > NewInputLatencyInfo() into ui_latency (it just adds RWH_COMPONENT), but
since
> we
> > are using NewInputLatencyInfo() everywhere, I feel it is better just call
> > NewInputLatencyInfo() and merge the result with ui_latency. 
> 
> Ok, I see.  But now that you're only conditionally merging ui_latency into
> latency_info, doesn't that mean in the non-gesture cases the data in
ui_latency
> is getting dropped on the floor?  So I think you at least want to merge it in
> outside the if block.

Doh', missed that....

> 
> It would be clearer to me if we refactored NewInputLatencyInfo to a function
> that adds the RWH_COMPONENT to an existing latency info so we're always either
> creating or extending a LatencyInfo.

Done.

> 
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host_impl.cc:1140: //
> INPUT_EVENT_LATENCY_RWH_COMPONENT right here.
> We should probably work to unify Android and Aura here.  Please file a bug to
> say Android should be propagating the latency_info for a touch event into the
> gesture event.  If you'd like to try putting up a CL for that, that's great,
> otherwise we can ask klobag@/aelias@ if someone on the Clank team wants to do
> it.
> 

bug filed  https://code.google.com/p/chromium/issues/detail?id=258976

> These numbers will be most useful when they have a consistent meaning across
all
> platforms...

[Rick Byers, 2013-07-10 17:20:42]

On 2013/07/10 16:34:07, Yufeng Shen wrote:
> On 2013/07/10 15:31:35, Rick Byers wrote:
> >
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> > content/browser/renderer_host/render_widget_host_impl.cc:1154:
> > latency_info.MergeWith(ui_latency);
> > On 2013/07/10 15:12:04, Yufeng Shen wrote:
> > > On 2013/07/10 01:36:30, Rick Byers wrote:
> > > > Why do we need two different LatencyInfo objects here?  Conceptually I
> would
> > > > have expected that we'd just call ui_latency.AddLatencyNumber and not
need
> > > > latency_info at all...
> > > 
> > > right. it is just that it happens to be coded this way. Originally without
> > > ui_latency, there is only one latency_info = NewInputLatencyInfo() is
> created
> > > here and passed on. Then there comes the ui_latency, and we need to merge
> what
> > > is in NewInputLatencyInfo() into ui_latency. We can directly add what's
> added
> > in
> > > NewInputLatencyInfo() into ui_latency (it just adds RWH_COMPONENT), but
> since
> > we
> > > are using NewInputLatencyInfo() everywhere, I feel it is better just call
> > > NewInputLatencyInfo() and merge the result with ui_latency. 
> > 
> > Ok, I see.  But now that you're only conditionally merging ui_latency into
> > latency_info, doesn't that mean in the non-gesture cases the data in
> ui_latency
> > is getting dropped on the floor?  So I think you at least want to merge it
in
> > outside the if block.
> 
> Doh', missed that....
> 
> > 
> > It would be clearer to me if we refactored NewInputLatencyInfo to a function
> > that adds the RWH_COMPONENT to an existing latency info so we're always
either
> > creating or extending a LatencyInfo.
> 
> Done.

I still find it confusing that NewInputLatencyInfo does two jobs (add / update)
depending on how it's used.  Why not just have two functions:

void AddInputInputLatencyInfo(LatencyInfo* info) { ... }

LatencyInfo NewInputLatencyInfo() {
  LatencyInfo info;
  AddInputLatencyInfo(&info);
  return info;
}

> 
> > 
> >
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> > content/browser/renderer_host/render_widget_host_impl.cc:1140: //
> > INPUT_EVENT_LATENCY_RWH_COMPONENT right here.
> > We should probably work to unify Android and Aura here.  Please file a bug
to
> > say Android should be propagating the latency_info for a touch event into
the
> > gesture event.  If you'd like to try putting up a CL for that, that's great,
> > otherwise we can ask klobag@/aelias@ if someone on the Clank team wants to
do
> > it.
> > 
> 
> bug filed  https://code.google.com/p/chromium/issues/detail?id=258976

Thanks!

> 
> > These numbers will be most useful when they have a consistent meaning across
> all
> > platforms...

[Rick Byers, 2013-07-10 17:27:14]

https://codereview.chromium.org/18937002/diff/10002/ui/base/latency_info.h
File ui/base/latency_info.h (right):

https://codereview.chromium.org/18937002/diff/10002/ui/base/latency_info.h#ne...
ui/base/latency_info.h:26: INPUT_EVENT_LATENCY_GESTURE_SCROLL_RWH_COMPONENT,
By the way, I'm guessing you'll eventually want to track GestureScrollStart and
GestureScrollUpdate separately since you'll be fixing the performance of
GestureScrollUpdate but not GestureScrollStart.

Eg. if a particular gesture has say a 1000ms latency for start, and then 100
updates each with 10ms latency, reporting that as an average of ~20ms per event
(or even a histogram with 99% <=10ms) hides the problem of a 1s start delay.

But doing that in a later CL makes sense.

[Yufeng Shen (Slow to review), 2013-07-10 17:55:28]

On 2013/07/10 17:20:42, Rick Byers wrote:
> On 2013/07/10 16:34:07, Yufeng Shen wrote:
> > On 2013/07/10 15:31:35, Rick Byers wrote:
> > >
> >
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> > > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> > > content/browser/renderer_host/render_widget_host_impl.cc:1154:
> > > latency_info.MergeWith(ui_latency);
> > > On 2013/07/10 15:12:04, Yufeng Shen wrote:
> > > > On 2013/07/10 01:36:30, Rick Byers wrote:
> > > > > Why do we need two different LatencyInfo objects here?  Conceptually I
> > would
> > > > > have expected that we'd just call ui_latency.AddLatencyNumber and not
> need
> > > > > latency_info at all...
> > > > 
> > > > right. it is just that it happens to be coded this way. Originally
without
> > > > ui_latency, there is only one latency_info = NewInputLatencyInfo() is
> > created
> > > > here and passed on. Then there comes the ui_latency, and we need to
merge
> > what
> > > > is in NewInputLatencyInfo() into ui_latency. We can directly add what's
> > added
> > > in
> > > > NewInputLatencyInfo() into ui_latency (it just adds RWH_COMPONENT), but
> > since
> > > we
> > > > are using NewInputLatencyInfo() everywhere, I feel it is better just
call
> > > > NewInputLatencyInfo() and merge the result with ui_latency. 
> > > 
> > > Ok, I see.  But now that you're only conditionally merging ui_latency into
> > > latency_info, doesn't that mean in the non-gesture cases the data in
> > ui_latency
> > > is getting dropped on the floor?  So I think you at least want to merge it
> in
> > > outside the if block.
> > 
> > Doh', missed that....
> > 
> > > 
> > > It would be clearer to me if we refactored NewInputLatencyInfo to a
function
> > > that adds the RWH_COMPONENT to an existing latency info so we're always
> either
> > > creating or extending a LatencyInfo.
> > 
> > Done.
> 
> I still find it confusing that NewInputLatencyInfo does two jobs (add /
update)
> depending on how it's used.  Why not just have two functions:
> 
> void AddInputInputLatencyInfo(LatencyInfo* info) { ... }
> 
> LatencyInfo NewInputLatencyInfo() {
>   LatencyInfo info;
>   AddInputLatencyInfo(&info);
>   return info;
> }
> 

In this case
AddInputLatencyInfo() would be just one call on info

void AddInputLatencyInfo(LatencyInfo* info) {
    info->AddLatency(...)
}

seems unnecessary.

I think the confusing comes from the name of NewInputLatencyInfo().
How about just rename it as CreateRWHLatencyInfoIfNotExist() ?
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> > > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> > > content/browser/renderer_host/render_widget_host_impl.cc:1140: //
> > > INPUT_EVENT_LATENCY_RWH_COMPONENT right here.
> > > We should probably work to unify Android and Aura here.  Please file a bug
> to
> > > say Android should be propagating the latency_info for a touch event into
> the
> > > gesture event.  If you'd like to try putting up a CL for that, that's
great,
> > > otherwise we can ask klobag@/aelias@ if someone on the Clank team wants to
> do
> > > it.
> > > 
> > 
> > bug filed  https://code.google.com/p/chromium/issues/detail?id=258976
> 
> Thanks!
> 
> > 
> > > These numbers will be most useful when they have a consistent meaning
across
> > all
> > > platforms...

[Rick Byers, 2013-07-10 19:18:02]

On 2013/07/10 17:55:28, Yufeng Shen wrote:
> On 2013/07/10 17:20:42, Rick Byers wrote:
> > On 2013/07/10 16:34:07, Yufeng Shen wrote:
> > > On 2013/07/10 15:31:35, Rick Byers wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> > > > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18937002/diff/1/content/browser/renderer_host...
> > > > content/browser/renderer_host/render_widget_host_impl.cc:1154:
> > > > latency_info.MergeWith(ui_latency);
> > > > On 2013/07/10 15:12:04, Yufeng Shen wrote:
> > > > > On 2013/07/10 01:36:30, Rick Byers wrote:
> > > > > > Why do we need two different LatencyInfo objects here?  Conceptually
I
> > > would
> > > > > > have expected that we'd just call ui_latency.AddLatencyNumber and
not
> > need
> > > > > > latency_info at all...
> > > > > 
> > > > > right. it is just that it happens to be coded this way. Originally
> without
> > > > > ui_latency, there is only one latency_info = NewInputLatencyInfo() is
> > > created
> > > > > here and passed on. Then there comes the ui_latency, and we need to
> merge
> > > what
> > > > > is in NewInputLatencyInfo() into ui_latency. We can directly add
what's
> > > added
> > > > in
> > > > > NewInputLatencyInfo() into ui_latency (it just adds RWH_COMPONENT),
but
> > > since
> > > > we
> > > > > are using NewInputLatencyInfo() everywhere, I feel it is better just
> call
> > > > > NewInputLatencyInfo() and merge the result with ui_latency. 
> > > > 
> > > > Ok, I see.  But now that you're only conditionally merging ui_latency
into
> > > > latency_info, doesn't that mean in the non-gesture cases the data in
> > > ui_latency
> > > > is getting dropped on the floor?  So I think you at least want to merge
it
> > in
> > > > outside the if block.
> > > 
> > > Doh', missed that....
> > > 
> > > > 
> > > > It would be clearer to me if we refactored NewInputLatencyInfo to a
> function
> > > > that adds the RWH_COMPONENT to an existing latency info so we're always
> > either
> > > > creating or extending a LatencyInfo.
> > > 
> > > Done.
> > 
> > I still find it confusing that NewInputLatencyInfo does two jobs (add /
> update)
> > depending on how it's used.  Why not just have two functions:
> > 
> > void AddInputInputLatencyInfo(LatencyInfo* info) { ... }
> > 
> > LatencyInfo NewInputLatencyInfo() {
> >   LatencyInfo info;
> >   AddInputLatencyInfo(&info);
> >   return info;
> > }
> > 
> 
> In this case
> AddInputLatencyInfo() would be just one call on info
> 
> void AddInputLatencyInfo(LatencyInfo* info) {
>     info->AddLatency(...)
> }
> 
> seems unnecessary.

If it's too trivial to be it's own function, then you could just put it in-line
the one place you need it rather than trying to call a function that's intended
for a slightly different scenario.

> I think the confusing comes from the name of NewInputLatencyInfo().
> How about just rename it as CreateRWHLatencyInfoIfNotExist() ?

Yeah that's probably good enough.  Not worth spending a ton of time debating
IMHO.

> > > 
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> > > > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18937002/diff/6001/content/browser/renderer_h...
> > > > content/browser/renderer_host/render_widget_host_impl.cc:1140: //
> > > > INPUT_EVENT_LATENCY_RWH_COMPONENT right here.
> > > > We should probably work to unify Android and Aura here.  Please file a
bug
> > to
> > > > say Android should be propagating the latency_info for a touch event
into
> > the
> > > > gesture event.  If you'd like to try putting up a CL for that, that's
> great,
> > > > otherwise we can ask klobag@/aelias@ if someone on the Clank team wants
to
> > do
> > > > it.
> > > > 
> > > 
> > > bug filed  https://code.google.com/p/chromium/issues/detail?id=258976
> > 
> > Thanks!
> > 
> > > 
> > > > These numbers will be most useful when they have a consistent meaning
> across
> > > all
> > > > platforms...

[Yufeng Shen (Slow to review), 2013-07-10 19:38:23]

rename NewInputLatencyInfo() to CreateRWHLatencyInfoIfNotExist() to be more
clear.

https://codereview.chromium.org/18937002/diff/10002/ui/base/latency_info.h
File ui/base/latency_info.h (right):

https://codereview.chromium.org/18937002/diff/10002/ui/base/latency_info.h#ne...
ui/base/latency_info.h:26: INPUT_EVENT_LATENCY_GESTURE_SCROLL_RWH_COMPONENT,
On 2013/07/10 17:27:14, Rick Byers wrote:
> By the way, I'm guessing you'll eventually want to track GestureScrollStart
and
> GestureScrollUpdate separately since you'll be fixing the performance of
> GestureScrollUpdate but not GestureScrollStart.
> 
> Eg. if a particular gesture has say a 1000ms latency for start, and then 100
> updates each with 10ms latency, reporting that as an average of ~20ms per
event
> (or even a histogram with 99% <=10ms) hides the problem of a 1s start delay.
> 
> But doing that in a later CL makes sense.

right, changed to add SCROLL_UPDATE_RWH_COMPONENT only to GestureScrollUpdate

[Rick Byers, 2013-07-10 20:19:03]

LGTM with nit

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

https://codereview.chromium.org/18937002/diff/19001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
"Event.Latency.Browser.TouchToScroll",
nit: probably want to call this TouchToScrollUpdate now to avoid confusion with
whatever stat you add for ScrollStart.

[Yufeng Shen (Slow to review), 2013-07-10 21:05:52]

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

https://codereview.chromium.org/18937002/diff/19001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
"Event.Latency.Browser.TouchToScroll",
On 2013/07/10 20:19:03, Rick Byers wrote:
> nit: probably want to call this TouchToScrollUpdate now to avoid confusion
with
> whatever stat you add for ScrollStart.

done. and also rename the telemetry stats to average_scroll_update_latency.

[sadrul, 2013-07-11 08:22:06]

Just some nits

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

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1143: type ==
WebKit::WebInputEvent::GestureScrollUpdateWithoutPropagation) {
I believe the WithoutPropagation version of the event should never arrive at the
browser. So this shouldn't be necessary.

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
"Event.Latency.Browser.TouchToScrollUpdate",
bikeshed: I don't think 'Browser.' means anything here anymore.

Can 'TouchToScrollUpdate' be confused to mean the latency from touch-event to
generation of scroll-update event?

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2696:
original_component.event_count *
2 more space indent

[Rick Byers, 2013-07-11 14:32:17]

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

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
"Event.Latency.Browser.TouchToScrollUpdate",
On 2013/07/11 08:22:06, sadrul wrote:
> bikeshed: I don't think 'Browser.' means anything here anymore.
> 
> Can 'TouchToScrollUpdate' be confused to mean the latency from touch-event to
> generation of scroll-update event?

Yeah, maybe Event.Latency.TouchToScrollUpdateSwap or something like that.

[Yufeng Shen (Slow to review), 2013-07-11 14:47:56]

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

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1143: type ==
WebKit::WebInputEvent::GestureScrollUpdateWithoutPropagation) {
On 2013/07/11 08:22:06, sadrul wrote:
> I believe the WithoutPropagation version of the event should never arrive at
the
> browser. So this shouldn't be necessary.

Done.

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
"Event.Latency.Browser.TouchToScrollUpdate",
On 2013/07/11 14:32:18, Rick Byers wrote:
> On 2013/07/11 08:22:06, sadrul wrote:
> > bikeshed: I don't think 'Browser.' means anything here anymore.
> > 
> > Can 'TouchToScrollUpdate' be confused to mean the latency from touch-event
to
> > generation of scroll-update event?
> 
> Yeah, maybe Event.Latency.TouchToScrollUpdateSwap or something like that.

Done.

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
"Event.Latency.Browser.TouchToScrollUpdate",
On 2013/07/11 08:22:06, sadrul wrote:
> bikeshed: I don't think 'Browser.' means anything here anymore.
> 
> Can 'TouchToScrollUpdate' be confused to mean the latency from touch-event to
> generation of scroll-update event?

Done.

https://codereview.chromium.org/18937002/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2696:
original_component.event_count *
On 2013/07/11 08:22:06, sadrul wrote:
> 2 more space indent

Done.

[jbauman, 2013-07-12 00:50:38]

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

https://codereview.chromium.org/18937002/diff/35001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1142: if (type ==
WebKit::WebInputEvent::GestureScrollUpdate) {
Would it make sense to copy the original timestamp from the
ui::INPUT_EVENT_LATENCY_ORIGINAL_COMPONENT from this LatencyInfo into the
timestamp for the ui::INPUT_EVENT_LATENCY_SCROLL_UPDATE_RWH_COMPONENT, so as to
allow it to track the latency from only these events?

https://codereview.chromium.org/18937002/diff/35001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
delta.InMicroseconds(),
So this measures the latency from all the input events in a frame if that frame
includes at least one scroll update, right?

[Yufeng Shen (Slow to review), 2013-07-12 21:12:37]

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

https://codereview.chromium.org/18937002/diff/35001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1142: if (type ==
WebKit::WebInputEvent::GestureScrollUpdate) {
On 2013/07/12 00:50:39, jbauman wrote:
> Would it make sense to copy the original timestamp from the
> ui::INPUT_EVENT_LATENCY_ORIGINAL_COMPONENT from this LatencyInfo into the
> timestamp for the ui::INPUT_EVENT_LATENCY_SCROLL_UPDATE_RWH_COMPONENT, so as
to
> allow it to track the latency from only these events?

Added a SCROLL_UPDATE_ORIGINAL_COMPONENT copied from ORIGINAL_COMPONENT so we
can specifically track the latency of scroll update.

https://codereview.chromium.org/18937002/diff/35001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:2689:
delta.InMicroseconds(),
On 2013/07/12 00:50:39, jbauman wrote:
> So this measures the latency from all the input events in a frame if that
frame
> includes at least one scroll update, right?

solved by using SCROLL_UPDATE_ORIGINAL component.

[jbauman, 2013-07-12 22:02:29]

lgtm. Thanks for changing that. I think it makes the code a bit more
straightforward.

[Yufeng Shen (Slow to review), 2013-07-15 16:12:11]

Ping 
sadrul@ for OWNER of content/browser/renderer_host/
+ jln@ for OWNER of content/common/
+ dtu@ for OWNER of tools/perf/measurements/

[sadrul, 2013-07-15 16:16:36]

LGTM

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

https://codereview.chromium.org/18937002/diff/40001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1144: if (type ==
WebKit::WebInputEvent::GestureScrollUpdate) {
Since |type| isn't used elsewhere, perhaps just 'if (gesture_event.type == ...)
{'?

[jln (very slow on Chromium), 2013-07-15 17:48:15]

> + jln@ for OWNER of content/common/
content/common/view_messages.h lgtm

for the rest of content/common, you need a content/ owner.

[dtu, 2013-07-15 19:18:46]

> + dtu@ for OWNER of tools/perf/measurements/

lgtm for tools/perf/

[commit-bot: I haz the power, 2013-07-15 19:33:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/18937002/48001

[commit-bot: I haz the power, 2013-07-15 21:51:03]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-15 21:57:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/18937002/48001

[commit-bot: I haz the power, 2013-07-15 23:44:28]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 15:09:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/18937002/48001

[commit-bot: I haz the power, 2013-07-16 16:21:59]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 16:38:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/18937002/48001

[commit-bot: I haz the power, 2013-07-16 17:55:18]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 21:44:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/18937002/48001

[commit-bot: I haz the power, 2013-07-16 23:35:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-17 03:06:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/18937002/48001

[commit-bot: I haz the power, 2013-07-17 05:12:18]

Change committed as 211948