Add DisplayPlacement/Layout mojoms + StructTraits.

Add DisplayPlacement/Layout mojoms + StructTraits.

Add mojoms and StructTraits so that a complete DisplayLayout can be
serialized. This includes DisplayPlacement and some enums. DisplayLayout
is serialized via unique_ptr since it is normally stored as a
unique_ptr.

This is required to pass the DisplayLayout from mus-ws to chrome for the
display options page.

BUG=686848
Review-Url: https://codereview.chromium.org/2661663002
Cr-Commit-Position: refs/heads/master@{#447990}
Committed: https://chromium.googlesource.com/chromium/src/+/1d6d5d29921ee1e7cc9c6ce11187bc8ccb11d8f3

[kylechar, 2017-02-01 19:44:49]

Patchset #3 (id:40001) has been deleted

[kylechar, 2017-02-01 19:49:43]

kylechar@chromium.org changed reviewers:
+ derat@chromium.org, tsepez@chromium.org

[kylechar, 2017-02-01 19:49:44]

derat: ui/display/* (except mojom/StructTraits)
tsepez: mojom/StructTraits

[kylechar, 2017-02-01 19:58:05]

Description was changed from

==========
Add DisplayPlacement/Layout mojoms + StructTraits.

Add mojoms and StructTraits so that a complete DisplayLayout can be
serialized. This includes DisplayPlacement and some enums. DisplayLayout
is serialized via unique_ptr since it is normally stored as a
unique_ptr.

This is required to pass the DisplayLayout from mus-ws to chrome for the
display options page.

BUG=TBD
==========

to

==========
Add DisplayPlacement/Layout mojoms + StructTraits.

Add mojoms and StructTraits so that a complete DisplayLayout can be
serialized. This includes DisplayPlacement and some enums. DisplayLayout
is serialized via unique_ptr since it is normally stored as a
unique_ptr.

This is required to pass the DisplayLayout from mus-ws to chrome for the
display options page.

BUG=686848
==========

[Tom Sepez, 2017-02-01 20:10:52]

https://codereview.chromium.org/2661663002/diff/80001/ui/display/mojo/display...
File ui/display/mojo/display_layout_struct_traits.cc (right):

https://codereview.chromium.org/2661663002/diff/80001/ui/display/mojo/display...
ui/display/mojo/display_layout_struct_traits.cc:97: display::DisplayLayout*
out_ptr = (*out).get();
nit: out->get()

https://codereview.chromium.org/2661663002/diff/80001/ui/display/mojo/display...
ui/display/mojo/display_layout_struct_traits.cc:100: return false;
we leak if we come here.  Better to hold the unique_ptr in a local and then
std::move it to *out at the end.

[kylechar, 2017-02-01 20:53:27]

https://codereview.chromium.org/2661663002/diff/80001/ui/display/mojo/display...
File ui/display/mojo/display_layout_struct_traits.cc (right):

https://codereview.chromium.org/2661663002/diff/80001/ui/display/mojo/display...
ui/display/mojo/display_layout_struct_traits.cc:97: display::DisplayLayout*
out_ptr = (*out).get();
On 2017/02/01 20:10:51, Tom Sepez wrote:
> nit: out->get()

Done.

https://codereview.chromium.org/2661663002/diff/80001/ui/display/mojo/display...
ui/display/mojo/display_layout_struct_traits.cc:100: return false;
On 2017/02/01 20:10:51, Tom Sepez wrote:
> we leak if we come here.  Better to hold the unique_ptr in a local and then
> std::move it to *out at the end.

Done.

[Tom Sepez, 2017-02-01 22:19:13]

lgtm

[Daniel Erat, 2017-02-01 23:06:37]

lgtm with a few comments

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
File ui/display/display_layout.cc (right):

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
ui/display/display_layout.cc:357: return display_id == other.display_id &&      
         //
what are all these empty trailing comments for?

https://codereview.chromium.org/2661663002/diff/100001/ui/display/mojo/displa...
File ui/display/mojo/display_layout.mojom (right):

https://codereview.chromium.org/2661663002/diff/100001/ui/display/mojo/displa...
ui/display/mojo/display_layout.mojom:12: LEFT
nit: mojo permits a trailing comma here and in OffsetReference, right?

[kylechar, 2017-02-02 14:40:48]

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
File ui/display/display_layout.cc (right):

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
ui/display/display_layout.cc:357: return display_id == other.display_id &&      
         //
On 2017/02/01 23:06:37, Daniel Erat wrote:
> what are all these empty trailing comments for?

Just to keep the statements one per line after running 'git cl format'.

It feels easier to read with one == per line and easier to see if someone adds a
member without updating the operator==. I can remove the comments if you don't
feel it's necessary.

https://codereview.chromium.org/2661663002/diff/100001/ui/display/mojo/displa...
File ui/display/mojo/display_layout.mojom (right):

https://codereview.chromium.org/2661663002/diff/100001/ui/display/mojo/displa...
ui/display/mojo/display_layout.mojom:12: LEFT
On 2017/02/01 23:06:37, Daniel Erat wrote:
> nit: mojo permits a trailing comma here and in OffsetReference, right?

Done.

[Daniel Erat, 2017-02-02 16:55:13]

lgtm

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
File ui/display/display_layout.cc (right):

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
ui/display/display_layout.cc:357: return display_id == other.display_id &&      
         //
On 2017/02/02 14:40:48, kylechar wrote:
> On 2017/02/01 23:06:37, Daniel Erat wrote:
> > what are all these empty trailing comments for?
> 
> Just to keep the statements one per line after running 'git cl format'.
> 
> It feels easier to read with one == per line and easier to see if someone adds
a
> member without updating the operator==. I can remove the comments if you don't
> feel it's necessary.

ah, i didn't know that one could do this (or that clang-format would try to wrap
expressions like this). it looks like it's already done in a fair number of
other places in the codebase, although i'm not sure that i see it for other
sequences of == expressions joined by &&.

i won't argue against this here if clang-format does something ugly otherwise,
but would you mind filing a request to make clang-format handle this better, per
the instructions at the bottom of
https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md, if
that hasn't been done already? initialization lists and argument lists already
use one-per-line in some cases, and it feels like it makes long strings of && or
|| expressions easier to read as well.

[kylechar, 2017-02-02 21:17:18]

Thanks!

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
File ui/display/display_layout.cc (right):

https://codereview.chromium.org/2661663002/diff/100001/ui/display/display_lay...
ui/display/display_layout.cc:357: return display_id == other.display_id &&      
         //
On 2017/02/02 16:55:13, Daniel Erat wrote:
> On 2017/02/02 14:40:48, kylechar wrote:
> > On 2017/02/01 23:06:37, Daniel Erat wrote:
> > > what are all these empty trailing comments for?
> > 
> > Just to keep the statements one per line after running 'git cl format'.
> > 
> > It feels easier to read with one == per line and easier to see if someone
adds
> a
> > member without updating the operator==. I can remove the comments if you
don't
> > feel it's necessary.
> 
> ah, i didn't know that one could do this (or that clang-format would try to
wrap
> expressions like this). it looks like it's already done in a fair number of
> other places in the codebase, although i'm not sure that i see it for other
> sequences of == expressions joined by &&.
> 
> i won't argue against this here if clang-format does something ugly otherwise,
> but would you mind filing a request to make clang-format handle this better,
per
> the instructions at the bottom of
> https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md,
if
> that hasn't been done already? initialization lists and argument lists already
> use one-per-line in some cases, and it feels like it makes long strings of &&
or
> || expressions easier to read as well.

On second though, for consistency with existing code I've removed the empty
comments. I filed crbug.com/688111 to see if one statement per line is possible
with clang-format. I think it helps, at least.

[kylechar, 2017-02-02 21:17:32]

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

[commit-bot: I haz the power, 2017-02-02 21:18:07]

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

[Daniel Erat, 2017-02-02 21:58:54]

thanks for filing the clang-format bug! still lgtm

[commit-bot: I haz the power, 2017-02-02 22:51:24]

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

[commit-bot: I haz the power, 2017-02-02 22:51:25]

Dry run: This issue passed the CQ dry run.

[kylechar, 2017-02-03 14:02:51]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2017-02-03 14:02:51]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2661663002/#ps140001
(title: "Remove empty comments.")

[commit-bot: I haz the power, 2017-02-03 14:03:09]

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, 2017-02-03 14:34:53]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486130571138330,
"parent_rev": "f0e9b0e1c113c2df05b2c6c71c6f4507d95a9415", "commit_rev":
"1d6d5d29921ee1e7cc9c6ce11187bc8ccb11d8f3"}

[commit-bot: I haz the power, 2017-02-03 14:35:25]

Description was changed from

==========
Add DisplayPlacement/Layout mojoms + StructTraits.

Add mojoms and StructTraits so that a complete DisplayLayout can be
serialized. This includes DisplayPlacement and some enums. DisplayLayout
is serialized via unique_ptr since it is normally stored as a
unique_ptr.

This is required to pass the DisplayLayout from mus-ws to chrome for the
display options page.

BUG=686848
==========

to

==========
Add DisplayPlacement/Layout mojoms + StructTraits.

Add mojoms and StructTraits so that a complete DisplayLayout can be
serialized. This includes DisplayPlacement and some enums. DisplayLayout
is serialized via unique_ptr since it is normally stored as a
unique_ptr.

This is required to pass the DisplayLayout from mus-ws to chrome for the
display options page.

BUG=686848

Review-Url: https://codereview.chromium.org/2661663002
Cr-Commit-Position: refs/heads/master@{#447990}
Committed:
https://chromium.googlesource.com/chromium/src/+/1d6d5d29921ee1e7cc9c6ce11187...
==========

[commit-bot: I haz the power, 2017-02-03 14:35:26]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/1d6d5d29921ee1e7cc9c6ce11187...