Add input coordinate information to InputLatency

Add input coordinate information to InputLatency

The coordinates information will be used in trace-viewer to display input
coordinates within the chrome compositor view.

Original patch by Vicentiu Ciorbaru <cvicentiu@chromium.org>;

Committed: https://crrev.com/43c330f730f11a108f01fb2e6331f9e49694f788
Cr-Commit-Position: refs/heads/master@{#295993}

[Sami, 2014-09-17 17:30:51]

skyostil@chromium.org changed reviewers:
+ cvicentiu@chromium.org, jdduke@chromium.org, miletus@chromium.org

[Sami, 2014-09-17 17:32:37]

Here's Vicentiu's patch from https://codereview.chromium.org/541033003/ with
some additional tests plus using arrays of floats instead of vectors for IPC.
PTAL.

[jdduke (slow), 2014-09-17 17:43:34]

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.cc
File ui/events/latency_info.cc (right):

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.c...
ui/events/latency_info.cc:107:
scoped_refptr<base::debug::ConvertableToTraceFormat> AsTraceableData(
Just to be clear, this method is only called when tracing is explicitly
enabled/active?

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.h...
ui/events/latency_info.h:103: 
If we can't use gfx::PointF, would this benefit from

struct LatencyCoordinate {
  LatencyCoordinate();
  float x, y;
};

?

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.h...
ui/events/latency_info.h:160: LatencyMap latency_components;
Are all these members public to simplify serialization? Seems rather unfortunate
:(

[Yufeng Shen (Slow to review), 2014-09-17 18:27:53]

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.cc
File ui/events/latency_info.cc (right):

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.c...
ui/events/latency_info.cc:107:
scoped_refptr<base::debug::ConvertableToTraceFormat> AsTraceableData(
On 2014/09/17 17:43:34, jdduke wrote:
> Just to be clear, this method is only called when tracing is explicitly
> enabled/active?

It is called below as an argument in TRACE_EVENT_ASYNC_END1()
I think all the parameter evaluation for trace event is conditioned 
on the certain tracing category being enabled.

[Sami, 2014-09-18 14:52:37]

Thanks, another look please?

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.cc
File ui/events/latency_info.cc (right):

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.c...
ui/events/latency_info.cc:107:
scoped_refptr<base::debug::ConvertableToTraceFormat> AsTraceableData(
On 2014/09/17 18:27:53, Yufeng Shen wrote:
> On 2014/09/17 17:43:34, jdduke wrote:
> > Just to be clear, this method is only called when tracing is explicitly
> > enabled/active?
> 
> It is called below as an argument in TRACE_EVENT_ASYNC_END1()
> I think all the parameter evaluation for trace event is conditioned 
> on the certain tracing category being enabled.

That's right. When tracing is disabled this code does nothing.

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.h...
ui/events/latency_info.h:103: 
On 2014/09/17 17:43:34, jdduke wrote:
> If we can't use gfx::PointF, would this benefit from
> 
> struct LatencyCoordinate {
>   LatencyCoordinate();
>   float x, y;
> };
> 
> ?

Yes, that brings some well needed structure.

https://codereview.chromium.org/569653002/diff/20001/ui/events/latency_info.h...
ui/events/latency_info.h:160: LatencyMap latency_components;
On 2014/09/17 17:43:34, jdduke wrote:
> Are all these members public to simplify serialization? Seems rather
unfortunate
> :(

I guess that is the reason. Everything else mostly uses methods to access the
same data.

[Yufeng Shen (Slow to review), 2014-09-18 15:02:31]

lgtm

[Sami, 2014-09-18 15:45:27]

skyostil@chromium.org changed reviewers:
+ dcheng@chromium.org, sadrul@chromium.org

[Sami, 2014-09-18 15:45:29]

Thanks Yufeng. Adding some owners:

dcheng@chromium.org: ui/events/ipc

sadrul@chromium.org: content/browser/renderer_host

PTAL.

[Sami, 2014-09-19 13:22:07]

skyostil@chromium.org changed reviewers:
- dcheng@chromium.org

[Sami, 2014-09-19 13:22:49]

skyostil@chromium.org changed reviewers:
+ inferno@chromium.org

[Sami, 2014-09-19 13:22:49]

Looks like dcheng@ is out, so inferno@ could you please take over?

[inferno, 2014-09-19 14:35:00]

On 2014/09/19 13:22:49, Sami wrote:
> Looks like dcheng@ is out, so inferno@ could you please take over?

i dont do ipc reviews@, cc tsepez / kenrb

[Sami, 2014-09-19 14:43:24]

skyostil@chromium.org changed reviewers:
+ kenrb@chromium.org
- inferno@chromium.org

[Sami, 2014-09-19 14:43:39]

kenrb ^

[kenrb, 2014-09-19 16:35:41]

ipc lgtm

[sadrul, 2014-09-19 18:19:01]

I'll wait for jdduke@'s approval

[jdduke (slow), 2014-09-19 18:46:19]

content/browser/renderer_host lgtm.

As an aside, at some point, I think we should take a hard look at how
LatencyInfo is coupled with events, how it is updated at each stage of the
pipeline, and how its members are exposed.  I think we can arrive at something
simpler that doesn't require quite so much boilerplate, in turn reducing the
amount of LatencyInfo-related code in RenderWidgetHostImpl.

[sadrul, 2014-09-19 20:44:01]

lgtm

On 2014/09/19 18:46:19, jdduke wrote:
> content/browser/renderer_host lgtm.
> 
> As an aside, at some point, I think we should take a hard look at how
> LatencyInfo is coupled with events, how it is updated at each stage of the
> pipeline, and how its members are exposed.  I think we can arrive at something
> simpler that doesn't require quite so much boilerplate, in turn reducing the
> amount of LatencyInfo-related code in RenderWidgetHostImpl.

a lot of +1s

[Sami, 2014-09-22 10:05:10]

Thanks all!

On 2014/09/19 18:46:19, jdduke wrote:
> As an aside, at some point, I think we should take a hard look at how
> LatencyInfo is coupled with events, how it is updated at each stage of the
> pipeline, and how its members are exposed.  I think we can arrive at something
> simpler that doesn't require quite so much boilerplate, in turn reducing the
> amount of LatencyInfo-related code in RenderWidgetHostImpl.

Another hearty +1 for that. For instance I think there are plans to use
LatencyInfo for estimating the GPU thread frame duration for the cc scheduler.
It feels like there's a simpler and more general design hiding here that can
cater to all of these "vertical" use cases that want to look at the system as a
whole.

[Sami, 2014-09-22 10:05:19]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-22 10:05:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/569653002/40001

[commit-bot: I haz the power, 2014-09-22 10:51:12]

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

[commit-bot: I haz the power, 2014-09-22 10:51:13]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sami, 2014-09-22 10:58:25]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-22 10:58:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/569653002/60001

[commit-bot: I haz the power, 2014-09-22 12:21:43]

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

[commit-bot: I haz the power, 2014-09-22 12:21:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2014-09-22 15:55:32]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-09-22 15:56:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/569653002/80001

[commit-bot: I haz the power, 2014-09-22 16:49:20]

Committed patchset #5 (id:80001) as 1092db33f4f2f74d52993d2ac1e93ac8b9e50b27

[commit-bot: I haz the power, 2014-09-22 16:50:08]

Patchset 5 (id:??) landed as
https://crrev.com/43c330f730f11a108f01fb2e6331f9e49694f788
Cr-Commit-Position: refs/heads/master@{#295993}