Implement LatencyInfo StructTraits

Implement LatencyInfo StructTraits

This CL implements LatencyInfo mojom types, and associated
StructTraits for serialization and deserialization. This will be used
by cc::CompositorFrameMetadata and ultimately cc::CompositorFrame
serialization/deserialization over mojo.

BUG=611802
Committed: https://crrev.com/6b3ec8379d9e19ec9a379cba96f3e40569328961
Cr-Commit-Position: refs/heads/master@{#398211}

[Fady Samuel, 2016-06-05 19:03:00]

Description was changed from

==========
[WIP] Implement LatencyInfo StructTraits

BUG=
==========

to

==========
Implement LatencyInfo StructTraits

BUG=611802
==========

[Fady Samuel, 2016-06-05 19:04:35]

Description was changed from

==========
Implement LatencyInfo StructTraits

BUG=611802
==========

to

==========
Implement LatencyInfo StructTraits

This CL implements LatencyInfo mojom types, and associated 
StructTraits for serialization and deserialization. This will be used
by cc::CompositorFrameMetadata and ultimately cc::CompositorFrame
serialization/deserialization over mojo.

BUG=611802
==========

[Fady Samuel, 2016-06-05 19:05:29]

fsamuel@chromium.org changed reviewers:
+ sadrul@chromium.org, tsepez@chromium.org, yzshen@chromium.org

[Fady Samuel, 2016-06-05 19:05:30]

+sadrul@ for ui/ OWNER
+tsepez@ for ipc
+yzshen@ for feedback on StructTraits.

Thanks all!

[Fady Samuel, 2016-06-05 19:05:40]

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

[commit-bot: I haz the power, 2016-06-05 19:05:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2040733002/140001

[commit-bot: I haz the power, 2016-06-05 20:22:32]

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

[commit-bot: I haz the power, 2016-06-05 20:22:33]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-06-06 16:15:22]

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info.mojom (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info.mojom:119: uint32 input_coordinates_size;
this seems unnecessary because that information is already in
|iput_coordinates|.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info_struct_traits.cc (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.cc:185: return input.x;
optional: It seems fine to inline such short function bodies. It will save quite
a lot of typing

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.cc:367: out->input_coordinates_size_ =
data.input_coordinates_size();
It seems you need to test whether the size from |data| makes sense first.
Otherwise, if |data| has more input coordinate than kMaxInputCoordinates, it
will result in buffer overflow, right?

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info_struct_traits.h (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.h:30: struct
ArrayTraits<InputCoordinateArray> {
Now that we have seen C array used in multiple places, maybe we should extra the
array traits for a pre-allocated C array as a common utility?

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.h:65: static std::string
trace_name(const ui::LatencyInfo& info);
Getter for object fields will be called twice. Returning a value has unnecessary
costs.
Please return a ref for objects that are non-trivial to copy/construct.

If you need extra work to compute/setup the object that you want to return, such
as the latency_components() getter, you can save it in a context object, please
see here:
https://codesearch.chromium.org/chromium/src/mojo/public/cpp/bindings/struct_...

[sadrul, 2016-06-06 16:20:14]

https://codereview.chromium.org/2040733002/diff/140001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/BUILD.gn#new...
ui/events/BUILD.gn:371: "run_all_unittests.cc",
Perhaps instead of adding a new run_all_unittests.cc here, we could bring in
//mojo/edk/test:run_all_unittests instead?

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info.mojom (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info.mojom:113: };
gfx.mojom.PointF?

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info_struct_traits.cc (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.cc:177: }
This is somewhat unfortunate. Can we cast between the two types?

https://codereview.chromium.org/2040733002/diff/140001/ui/events/run_all_unit...
File ui/events/run_all_unittests.cc (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/run_all_unit...
ui/events/run_all_unittests.cc:28: #endif
Looks like //ios/web/test/run_all_unittests.cc calls mojo::edk::Init(). Perhaps
we don't need the OS_IOS check?

[Fady Samuel, 2016-06-06 18:00:23]

PTAL Yuzhu, Sadrul!

https://codereview.chromium.org/2040733002/diff/140001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/BUILD.gn#new...
ui/events/BUILD.gn:371: "run_all_unittests.cc",
On 2016/06/06 16:20:13, sadrul wrote:
> Perhaps instead of adding a new run_all_unittests.cc here, we could bring in
> //mojo/edk/test:run_all_unittests instead?

Done.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info.mojom (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info.mojom:113: };
On 2016/06/06 16:20:13, sadrul wrote:
> gfx.mojom.PointF?

LatencyInfo has an InputCoordinate struct so I mapped that directly here. I can
do the cleanup in a follow-on CL.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info.mojom:119: uint32 input_coordinates_size;
On 2016/06/06 16:15:22, yzshen1 wrote:
> this seems unnecessary because that information is already in
> |iput_coordinates|.

Done.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info_struct_traits.cc (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.cc:177: }
On 2016/06/06 16:20:13, sadrul wrote:
> This is somewhat unfortunate. Can we cast between the two types?

We decided that this is the best way to catch changes to enums across the C++
version of mojom version.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.cc:185: return input.x;
On 2016/06/06 16:15:22, yzshen1 wrote:
> optional: It seems fine to inline such short function bodies. It will save
quite
> a lot of typing

I inlined InputCoordinate. Done.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.cc:367: out->input_coordinates_size_ =
data.input_coordinates_size();
On 2016/06/06 16:15:22, yzshen1 wrote:
> It seems you need to test whether the size from |data| makes sense first.
> Otherwise, if |data| has more input coordinate than kMaxInputCoordinates, it
> will result in buffer overflow, right?

Done.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
File ui/events/mojo/latency_info_struct_traits.h (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.h:30: struct
ArrayTraits<InputCoordinateArray> {
On 2016/06/06 16:15:22, yzshen1 wrote:
> Now that we have seen C array used in multiple places, maybe we should extra
the
> array traits for a pre-allocated C array as a common utility?

I'll do this in a follow-on CL. I added a TODO. Thanks!

https://codereview.chromium.org/2040733002/diff/140001/ui/events/mojo/latency...
ui/events/mojo/latency_info_struct_traits.h:65: static std::string
trace_name(const ui::LatencyInfo& info);
On 2016/06/06 16:15:22, yzshen1 wrote:
> Getter for object fields will be called twice. Returning a value has
unnecessary
> costs.
> Please return a ref for objects that are non-trivial to copy/construct.
> 
> If you need extra work to compute/setup the object that you want to return,
such
> as the latency_components() getter, you can save it in a context object,
please
> see here:
>
https://codesearch.chromium.org/chromium/src/mojo/public/cpp/bindings/struct_...

Done. Please let me know if I did this right for latency_components.

https://codereview.chromium.org/2040733002/diff/140001/ui/events/run_all_unit...
File ui/events/run_all_unittests.cc (right):

https://codereview.chromium.org/2040733002/diff/140001/ui/events/run_all_unit...
ui/events/run_all_unittests.cc:28: #endif
On 2016/06/06 16:20:13, sadrul wrote:
> Looks like //ios/web/test/run_all_unittests.cc calls mojo::edk::Init().
Perhaps
> we don't need the OS_IOS check?

I got rid of this file.

[Tom Sepez, 2016-06-06 18:08:38]

lgtm

[sadrul, 2016-06-06 18:56:03]

lgtm

https://codereview.chromium.org/2040733002/diff/160001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/2040733002/diff/160001/ui/events/BUILD.gn#new...
ui/events/BUILD.gn:405: "//mojo/public/cpp/bindings",
You might not need these anymore?

[yzshen1, 2016-06-06 19:38:29]

The Traits implementation LGTM

[Fady Samuel, 2016-06-06 19:44:48]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-06-06 19:45:12]

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

[commit-bot: I haz the power, 2016-06-06 21:01:08]

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

[commit-bot: I haz the power, 2016-06-06 21:01:09]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Fady Samuel, 2016-06-06 21:23:00]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-06 21:23:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2040733002/#ps200001
(title: "Hopefully better fix for windows")

[commit-bot: I haz the power, 2016-06-06 21:23:33]

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

[commit-bot: I haz the power, 2016-06-06 21:36:48]

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

[commit-bot: I haz the power, 2016-06-06 21:36:50]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Fady Samuel, 2016-06-06 22:03:41]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-06-06 22:04:18]

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

[commit-bot: I haz the power, 2016-06-06 22:21:03]

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

[commit-bot: I haz the power, 2016-06-06 22:21:05]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Fady Samuel, 2016-06-06 22:24:14]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-06 22:24:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2040733002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-06 22:24:59]

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

[Fady Samuel, 2016-06-06 22:35:13]

CQ'ed.

https://codereview.chromium.org/2040733002/diff/160001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/2040733002/diff/160001/ui/events/BUILD.gn#new...
ui/events/BUILD.gn:405: "//mojo/public/cpp/bindings",
On 2016/06/06 18:56:03, sadrul wrote:
> You might not need these anymore?

struct_traits_unittest is part of this target and uses mojo.

[Fady Samuel, 2016-06-06 23:17:18]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-06 23:17:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2040733002/#ps240001
(title: "Make InputCoordinateArray::size a size_t")

[commit-bot: I haz the power, 2016-06-06 23:17:49]

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

[Fady Samuel, 2016-06-07 00:16:06]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-07 00:16:07]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sadrul@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2040733002/#ps260001
(title: "Make Windows happier")

[commit-bot: I haz the power, 2016-06-07 00:16:41]

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

[commit-bot: I haz the power, 2016-06-07 02:55:59]

Description was changed from

==========
Implement LatencyInfo StructTraits

This CL implements LatencyInfo mojom types, and associated 
StructTraits for serialization and deserialization. This will be used
by cc::CompositorFrameMetadata and ultimately cc::CompositorFrame
serialization/deserialization over mojo.

BUG=611802
==========

to

==========
Implement LatencyInfo StructTraits

This CL implements LatencyInfo mojom types, and associated 
StructTraits for serialization and deserialization. This will be used
by cc::CompositorFrameMetadata and ultimately cc::CompositorFrame
serialization/deserialization over mojo.

BUG=611802
==========

[commit-bot: I haz the power, 2016-06-07 02:56:00]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-07 02:59:11]

Description was changed from

==========
Implement LatencyInfo StructTraits

This CL implements LatencyInfo mojom types, and associated 
StructTraits for serialization and deserialization. This will be used
by cc::CompositorFrameMetadata and ultimately cc::CompositorFrame
serialization/deserialization over mojo.

BUG=611802
==========

to

==========
Implement LatencyInfo StructTraits

This CL implements LatencyInfo mojom types, and associated
StructTraits for serialization and deserialization. This will be used
by cc::CompositorFrameMetadata and ultimately cc::CompositorFrame
serialization/deserialization over mojo.

BUG=611802

Committed: https://crrev.com/6b3ec8379d9e19ec9a379cba96f3e40569328961
Cr-Commit-Position: refs/heads/master@{#398211}
==========

[commit-bot: I haz the power, 2016-06-07 02:59:13]

Patchset 14 (id:??) landed as
https://crrev.com/6b3ec8379d9e19ec9a379cba96f3e40569328961
Cr-Commit-Position: refs/heads/master@{#398211}