Add a mojom/StructTrait for display::Display.

Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. Unit tests will be added to trybots in a
follow-up CL.

BUG=612242
Committed: https://crrev.com/97a62c38c3f2d818c24190d3495853c3de8c7273
Cr-Commit-Position: refs/heads/master@{#406319}

[kylechar, 2016-07-05 15:49:48]

Description was changed from

==========
Add a mojom/StructTrait for ui::Display.

There is currently a mojom that mostly corresponds to ui::Display and a
corresponding TypeConverter. The existing mojom is not usable outside of
mus and has extra fields.

This also adds unit tests to ensure StructTrait works correctly.

The new mojom and StructTraits isn't used in code but will be needed to
move ui::Display objects around in code that isn't mus specific.

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

to

==========
Add a mojom/StructTrait for ui::Display.

There is currently a mojom that mostly corresponds to ui::Display and a
corresponding TypeConverter. However, existing mojom has some extra
information that is mus specific. In the future ui.mojom.Display will be
modified to not duplicate fields.

Add new mojom and unit tests to ensure StructTrait works correctly.

The new mojom and StructTraits will be needed for the DisplayController
Mojo interface to allow display configuration to happen.

BUG=612242
==========

[kylechar, 2016-07-05 18:39:04]

Description was changed from

==========
Add a mojom/StructTrait for ui::Display.

There is currently a mojom that mostly corresponds to ui::Display and a
corresponding TypeConverter. However, existing mojom has some extra
information that is mus specific. In the future ui.mojom.Display will be
modified to not duplicate fields.

Add new mojom and unit tests to ensure StructTrait works correctly.

The new mojom and StructTraits will be needed for the DisplayController
Mojo interface to allow display configuration to happen.

BUG=612242
==========

to

==========
Add a mojom/StructTrait for ui::Display.

There is currently a mojom that mostly corresponds to ui::Display and a
corresponding TypeConverter. However, existing mojom has some extra
information that is mus specific. In the future ui.mojom.Display will be
modified to not duplicate fields.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

[kylechar, 2016-07-05 18:41:03]

kylechar@chromium.org changed reviewers:
+ sky@chromium.org

[kylechar, 2016-07-05 18:41:04]

+sky for basically everything. I needs security/mojo reviewers after.

[sky, 2016-07-06 16:40:34]

Also, if you really want a separate namespace I would be inclined to still keep
it in ui, eg ui.display.mojom. And, I think it should live in
services/ui/public/interfaces/display.

https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
File ui/display/mojo/display.mojom (right):

https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
ui/display/mojo/display.mojom:9: // Corresponds to display::Display::Rotation.
The reason I added FrameDecorationValues to Display is that nearly all mus
clients need to know the FrameDecorationValues so that they can create windows.
That said, I don't think we should create duplicate structures. Instead I
recommend moving FrameDecorationValues out of Display and have the function that
supplies FrameDecorationValues to provide it as an additional argument (or use a
struct with FrameDecorationValues and Display). That way we can have one display
class that is used by both.

[kylechar, 2016-07-06 17:19:20]

With the regards to the namespace, I was just mirroring the the namespace used
for ui/display/display.h.

Although if I move the mojom to services/ui/public/interfaces/display then I
agree in changing the namespace.

https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
File ui/display/mojo/display.mojom (right):

https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
ui/display/mojo/display.mojom:9: // Corresponds to display::Display::Rotation.
On 2016/07/06 16:40:34, sky wrote:
> The reason I added FrameDecorationValues to Display is that nearly all mus
> clients need to know the FrameDecorationValues so that they can create
windows.
> That said, I don't think we should create duplicate structures. Instead I
> recommend moving FrameDecorationValues out of Display and have the function
that
> supplies FrameDecorationValues to provide it as an additional argument (or use
a
> struct with FrameDecorationValues and Display). That way we can have one
display
> class that is used by both.

It's not really intended to keep duplicate structures, this is just a first to
add the 1:1 corresponding mojom. I don't think it's really possible to have
extra fields in a mojom and use a StructTraits, because those extra fields won't
be serialized/deserialized?

From here I could make a C++ MusDisplay (or something like that) class:

public MusDisplay : display::Display {
 public:
   // Details.
 private:
  FrameDecorationValues values;
}

And make a mojom struct:

struct MusDisplay {
  display.mojom.Display display;
  FrameDecorationvalues values;
}

The StructTraits for the mojom<->class is trivial at that point. Mus can then
use the new C++ class directly instead of the ui::mojom::DisplayPtr which should
make life a bit nicer.

[sky, 2016-07-06 19:46:13]

On Wed, Jul 6, 2016 at 10:19 AM,  <kylechar@chromium.org> wrote:
> With the regards to the namespace, I was just mirroring the the namespace
> used
> for ui/display/display.h.
>
> Although if I move the mojom to services/ui/public/interfaces/display then I
> agree in changing the namespace.

Ben has expressed a desire for all code to live under services/ui,
which is what I'm echoing.
>
>
>
https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
> File ui/display/mojo/display.mojom (right):
>
>
https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
> ui/display/mojo/display.mojom:9: // Corresponds to
> display::Display::Rotation.
> On 2016/07/06 16:40:34, sky wrote:
>> The reason I added FrameDecorationValues to Display is that nearly all
> mus
>> clients need to know the FrameDecorationValues so that they can create
> windows.
>> That said, I don't think we should create duplicate structures.
> Instead I
>> recommend moving FrameDecorationValues out of Display and have the
> function that
>> supplies FrameDecorationValues to provide it as an additional argument
> (or use a
>> struct with FrameDecorationValues and Display). That way we can have
> one display
>> class that is used by both.
>
> It's not really intended to keep duplicate structures, this is just a
> first to add the 1:1 corresponding mojom. I don't think it's really
> possible to have extra fields in a mojom and use a StructTraits, because
> those extra fields won't be serialized/deserialized?
>
> From here I could make a C++ MusDisplay (or something like that) class:
>
> public MusDisplay : display::Display {
> public:
> // Details.
> private:
> FrameDecorationValues values;
> }

Display isn't intended for subclassing, so that won't work. I also
don't think you want subclassing, rather than a class that has a
display and has framedecorationvalues.

> And make a mojom struct:
>
> struct MusDisplay {
> display.mojom.Display display;
> FrameDecorationvalues values;
> }
>
> The StructTraits for the mojom<->class is trivial at that point. Mus can
> then use the new C++ class directly instead of the ui::mojom::DisplayPtr
> which should make life a bit nicer.

This makes sense. I wouldn't name it MusDisplay.

  -Scott

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylechar, 2016-07-08 18:28:00]

On 2016/07/06 19:46:13, sky wrote:
> On Wed, Jul 6, 2016 at 10:19 AM,  <mailto:kylechar@chromium.org> wrote:
> > With the regards to the namespace, I was just mirroring the the namespace
> > used
> > for ui/display/display.h.
> >
> > Although if I move the mojom to services/ui/public/interfaces/display then I
> > agree in changing the namespace.
> 
> Ben has expressed a desire for all code to live under services/ui,
> which is what I'm echoing.
> >
> >
> >
>
https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
> > File ui/display/mojo/display.mojom (right):
> >
> >
>
https://codereview.chromium.org/2123613002/diff/40001/ui/display/mojo/display...
> > ui/display/mojo/display.mojom:9: // Corresponds to
> > display::Display::Rotation.
> > On 2016/07/06 16:40:34, sky wrote:
> >> The reason I added FrameDecorationValues to Display is that nearly all
> > mus
> >> clients need to know the FrameDecorationValues so that they can create
> > windows.
> >> That said, I don't think we should create duplicate structures.
> > Instead I
> >> recommend moving FrameDecorationValues out of Display and have the
> > function that
> >> supplies FrameDecorationValues to provide it as an additional argument
> > (or use a
> >> struct with FrameDecorationValues and Display). That way we can have
> > one display
> >> class that is used by both.
> >
> > It's not really intended to keep duplicate structures, this is just a
> > first to add the 1:1 corresponding mojom. I don't think it's really
> > possible to have extra fields in a mojom and use a StructTraits, because
> > those extra fields won't be serialized/deserialized?
> >
> > From here I could make a C++ MusDisplay (or something like that) class:
> >
> > public MusDisplay : display::Display {
> > public:
> > // Details.
> > private:
> > FrameDecorationValues values;
> > }
> 
> Display isn't intended for subclassing, so that won't work. I also
> don't think you want subclassing, rather than a class that has a
> display and has framedecorationvalues.
> 
> > And make a mojom struct:
> >
> > struct MusDisplay {
> > display.mojom.Display display;
> > FrameDecorationvalues values;
> > }
> >
> > The StructTraits for the mojom<->class is trivial at that point. Mus can
> > then use the new C++ class directly instead of the ui::mojom::DisplayPtr
> > which should make life a bit nicer.
> 
> This makes sense. I wouldn't name it MusDisplay.
> 
>   -Scott
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

That all sgtm. Have a few other display things to look at first and then I will
loop back around to this next week.

[kylechar, 2016-07-18 16:33:59]

Description was changed from

==========
Add a mojom/StructTrait for ui::Display.

There is currently a mojom that mostly corresponds to ui::Display and a
corresponding TypeConverter. However, existing mojom has some extra
information that is mus specific. In the future ui.mojom.Display will be
modified to not duplicate fields.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

to

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display::Display to avoid duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

[kylechar, 2016-07-18 19:57:27]

Description was changed from

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display::Display to avoid duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

to

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

[kylechar, 2016-07-18 20:51:36]

Description was changed from

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

to

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

[kylechar, 2016-07-18 20:58:30]

I've moved the mojom and StructTrait to services/ui/public/interfaces/display. A
few things though.

1. I left the mojom namespace as display.mojom.Display. There is already
ui.mojom.Display that will have to be renamed before I can reuse that.
2. I put the StructTraits and tests in the same directory as the mojom. There
are actually 0 StructTraits in services/ui/ right now so there isn't really any
precedent for where to put them. Most other StructTraits are in mojo/ or ipc/
directories alongside the mojoms.
3. The unit tests for the StructTraits need to be hooked up. Do we want have one
services/ui/public/interfaces test target to make it easy to add new
StructTraits unit tests? Or should there be one test target for each
subdirectory of services/ui/public/interfaces?

[sky, 2016-07-18 22:12:29]

On 2016/07/18 20:58:30, kylechar wrote:
> I've moved the mojom and StructTrait to services/ui/public/interfaces/display.
A
> few things though.
> 
> 1. I left the mojom namespace as display.mojom.Display. There is already
> ui.mojom.Display that will have to be renamed before I can reuse that.
> 2. I put the StructTraits and tests in the same directory as the mojom. There
> are actually 0 StructTraits in services/ui/ right now so there isn't really
any
> precedent for where to put them. Most other StructTraits are in mojo/ or ipc/
> directories alongside the mojoms.
> 3. The unit tests for the StructTraits need to be hooked up. Do we want have
one
> services/ui/public/interfaces test target to make it easy to add new
> StructTraits unit tests? Or should there be one test target for each
> subdirectory of services/ui/public/interfaces?

I'm ok with a single test target. We can restructure at the time a single test
target becomes too mush.

[sky, 2016-07-18 22:16:25]

LGTM - you'll need to get a security reviewer.

[kylechar, 2016-07-19 12:52:35]

Description was changed from

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. They're not used beyond tests at this point.

BUG=612242
==========

to

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. Unit tests will be added to trybots in a
follow-up CL.

BUG=612242
==========

[kylechar, 2016-07-19 12:53:19]

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

[kylechar, 2016-07-19 12:53:20]

+tsepez for security review.

[Tom Sepez, 2016-07-19 16:24:37]

mojom lgtm

[kylechar, 2016-07-19 16:59:44]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-07-19 16:59:45]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2123613002/#ps100001
(title: "Move unit test target.")

[commit-bot: I haz the power, 2016-07-19 17:00:49]

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, 2016-07-19 18:09:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-19 18:10:19]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 18:12:08]

Description was changed from

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. Unit tests will be added to trybots in a
follow-up CL.

BUG=612242
==========

to

==========
Add a mojom/StructTrait for display::Display.

There is currently a mojom that mostly corresponds to display::Display
and a corresponding TypeConverter. However, the existing mojom has some
extra information that is mus specific. In the future ui.mojom.Display
will be modified to contain a display.mojom.Display to avoid
duplication.

This CL only adds the new mojom, StructTrait and unit tests to ensure
StructTrait works correctly.  The new mojom and StructTrait will be
needed for the DisplayController Mojo interface to allow display
configuration to happen. Unit tests will be added to trybots in a
follow-up CL.

BUG=612242

Committed: https://crrev.com/97a62c38c3f2d818c24190d3495853c3de8c7273
Cr-Commit-Position: refs/heads/master@{#406319}
==========

[commit-bot: I haz the power, 2016-07-19 18:12:09]

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