[mojo-bindings] Support reuse of native enum classes

[mojo-bindings] Support reuse of native enum classes

This CL supports the Native=True attribute on empty
mojom enum declarations, allowing typemaps to establish
the mojom enum name as an alias for some native enum
class defined elsewhere.

Optional custom validation is supported via
specialization of mojo::EnumTraits<T>.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[Ken Rockot(use gerrit already), 2016-01-25 19:05:07]

rockot@chromium.org changed reviewers:
+ sky@chromium.org, yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-01-25 19:05:07]

Yuzhu, please take a look. I just realized you were going to look at enum
validation - hopefully this doesn't interfere. Seemed like low-hanging fruit and
a good precursor to converting more stuff to Mojo.

Scott, please take a look at Mus changes.

Thanks!

[sky, 2016-01-25 20:14:44]

nice, LGTM

[Ken Rockot(use gerrit already), 2016-01-25 21:20:21]

Description was changed from

==========
Let Mojo bindings reuse existing enum classes

This lets us declare a native enum in mojom with a
typemap pointing to the existing typename.

To support this a new explicit cpp_only option is
added to mojom rules.

mus::mojom::CommandBufferNamespace is converted to
an alias of gpu::CommandBufferNamespace as a
proof-of-concept.

BUG=404186
==========

to

==========
Let Mojo bindings reuse existing enum classes

This lets us declare a native enum in mojom with a
typemap pointing to the existing typename.

To support this a new explicit cpp_only option is
added to mojom rules.

mus::mojom::CommandBufferNamespace is converted to
an alias of gpu::CommandBufferNamespace as a
proof-of-concept, as well as
mus::mojom::ResourceFormat => cc::ResourceFormat

BUG=404186
==========

[yzshen1, 2016-01-26 04:36:30]

Only a few nits.

One question: As you can see in https://codereview.chromium.org/1618963006/
I changed the "wire type" of mojom enum FooEnum from uint32_t to FooEnum_Data,
in order to enable enum validation. For native only enums, I think it should
remain uint32_t?

https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
File components/mus/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
components/mus/public/interfaces/compositor_frame.mojom:20: [native=True]
Considering that we have MinVersion (and previously "Client"), I think this
should probably be "Native". (Sorry I didn't catch this in previous code
reviews.)

https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
File mandoline/ui/desktop_ui/BUILD.gn (right):

https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
mandoline/ui/desktop_ui/BUILD.gn:48: "public/interfaces",
nit: I think according to GN style guide, it is preferred to use a path starting
from '//', unless it is from the same file.

https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
(right):

https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:20: {%
endif %}
nit: "{%-" (and also line 54)

[yzshen1, 2016-01-26 04:37:33]

On 2016/01/26 04:36:30, yzshen1 wrote:
> Only a few nits.
> 
> One question: As you can see in https://codereview.chromium.org/1618963006/
> I changed the "wire type" of mojom enum FooEnum from uint32_t to FooEnum_Data,
^^^
I meant to say "I am going to change", either before or after this CL.

[Ken Rockot(use gerrit already), 2016-01-26 15:41:21]

I'll wait until you land your validation patch before rebasing this and fixing
nits.

On 2016/01/26 at 04:36:30, yzshen wrote:
> Only a few nits.
> 
> One question: As you can see in https://codereview.chromium.org/1618963006/
> I changed the "wire type" of mojom enum FooEnum from uint32_t to FooEnum_Data,
in order to enable enum validation. For native only enums, I think it should
remain uint32_t?
> 

SGTM

>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
> File components/mus/public/interfaces/compositor_frame.mojom (right):
> 
>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
> components/mus/public/interfaces/compositor_frame.mojom:20: [native=True]
> Considering that we have MinVersion (and previously "Client"), I think this
should probably be "Native". (Sorry I didn't catch this in previous code
reviews.)
> 

Yeah I saw your bug and you're right. I'll change that in a new CL before
landing this one.

>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
> File mandoline/ui/desktop_ui/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
> mandoline/ui/desktop_ui/BUILD.gn:48: "public/interfaces",
> nit: I think according to GN style guide, it is preferred to use a path
starting from '//', unless it is from the same file.
> 
>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
> File mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
(right):
> 
>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
> mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:20: {%
endif %}
> nit: "{%-" (and also line 54)

[yzshen1, 2016-01-26 17:02:22]

On Tue, Jan 26, 2016 at 7:41 AM, <rockot@chromium.org> wrote:

> I'll wait until you land your validation patch before rebasing this and
> fixing
> nits.


Thanks!

It would be nice to have some unit tests, too. :) (Either in this CL or in
a future CL.)


>
>
> On 2016/01/26 at 04:36:30, yzshen wrote:
>
>> Only a few nits.
>>
>
> One question: As you can see in
>> https://codereview.chromium.org/1618963006/
>> I changed the "wire type" of mojom enum FooEnum from uint32_t to
>> FooEnum_Data,
>>
> in order to enable enum validation. For native only enums, I think it
> should
> remain uint32_t?
>
>
> SGTM


Btw, are you planning to add validation for customized enums or would like
me to make the change?


>
>
>
>
>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
>
>> File components/mus/public/interfaces/compositor_frame.mojom (right):
>>
>
>
>
>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
>
>> components/mus/public/interfaces/compositor_frame.mojom:20: [native=True]
>> Considering that we have MinVersion (and previously "Client"), I think
>> this
>>
> should probably be "Native". (Sorry I didn't catch this in previous code
> reviews.)
>
>
> Yeah I saw your bug and you're right. I'll change that in a new CL before
> landing this one.
>
>
>
>
>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
>
>> File mandoline/ui/desktop_ui/BUILD.gn (right):
>>
>
>
>
>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
>
>> mandoline/ui/desktop_ui/BUILD.gn:48: "public/interfaces",
>> nit: I think according to GN style guide, it is preferred to use a path
>>
> starting from '//', unless it is from the same file.
>
>
>
>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
>
>> File mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
>>
> (right):
>
>
>
>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
>
>> mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:20:
>> {%
>>
> endif %}
>
>> nit: "{%-" (and also line 54)
>>
>
>
> https://codereview.chromium.org/1635613002/
>

-- 
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.

[Ken Rockot(use gerrit already), 2016-01-26 17:14:55]

On Tue, Jan 26, 2016 at 9:02 AM, Yuzhu Shen <yzshen@chromium.org> wrote:

>
>
> On Tue, Jan 26, 2016 at 7:41 AM, <rockot@chromium.org> wrote:
>
>> I'll wait until you land your validation patch before rebasing this and
>> fixing
>> nits.
>
>
> Thanks!
>
> It would be nice to have some unit tests, too. :) (Either in this CL or in
> a future CL.)
>

I'll add tests here :)


>
>
>>
>>
>> On 2016/01/26 at 04:36:30, yzshen wrote:
>>
>>> Only a few nits.
>>>
>>
>> One question: As you can see in
>>> https://codereview.chromium.org/1618963006/
>>> I changed the "wire type" of mojom enum FooEnum from uint32_t to
>>> FooEnum_Data,
>>>
>> in order to enable enum validation. For native only enums, I think it
>> should
>> remain uint32_t?
>>
>>
>> SGTM
>
>
> Btw, are you planning to add validation for customized enums or would like
> me to make the change?
>

I was considering it, but it's not clear to me what the right way to do
this is. One simple approach would be to provide a macro which can take an
arbitrary boolean expression and it defines some traits for you. Or we
could just have you provide your own validation traits. WDYT?


>
>
>>
>>
>>
>>
>>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
>>
>>> File components/mus/public/interfaces/compositor_frame.mojom (right):
>>>
>>
>>
>>
>>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
>>
>>> components/mus/public/interfaces/compositor_frame.mojom:20: [native=True]
>>> Considering that we have MinVersion (and previously "Client"), I think
>>> this
>>>
>> should probably be "Native". (Sorry I didn't catch this in previous code
>> reviews.)
>>
>>
>> Yeah I saw your bug and you're right. I'll change that in a new CL before
>> landing this one.
>>
>>
>>
>>
>>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
>>
>>> File mandoline/ui/desktop_ui/BUILD.gn (right):
>>>
>>
>>
>>
>>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
>>
>>> mandoline/ui/desktop_ui/BUILD.gn:48: "public/interfaces",
>>> nit: I think according to GN style guide, it is preferred to use a path
>>>
>> starting from '//', unless it is from the same file.
>>
>>
>>
>>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
>>
>>> File mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
>>>
>> (right):
>>
>>
>>
>>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
>>
>>> mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:20:
>>> {%
>>>
>> endif %}
>>
>>> nit: "{%-" (and also line 54)
>>>
>>
>>
>> https://codereview.chromium.org/1635613002/
>>
>
>

-- 
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.

[yzshen1, 2016-01-26 17:55:58]

On Tue, Jan 26, 2016 at 9:14 AM, Ken Rockot <rockot@chromium.org> wrote:

>
> On Tue, Jan 26, 2016 at 9:02 AM, Yuzhu Shen <yzshen@chromium.org> wrote:
>
>>
>>
>> On Tue, Jan 26, 2016 at 7:41 AM, <rockot@chromium.org> wrote:
>>
>>> I'll wait until you land your validation patch before rebasing this and
>>> fixing
>>> nits.
>>
>>
>> Thanks!
>>
>> It would be nice to have some unit tests, too. :) (Either in this CL or
>> in a future CL.)
>>
>
> I'll add tests here :)
>
>
>>
>>
>>>
>>>
>>> On 2016/01/26 at 04:36:30, yzshen wrote:
>>>
>>>> Only a few nits.
>>>>
>>>
>>> One question: As you can see in
>>>> https://codereview.chromium.org/1618963006/
>>>> I changed the "wire type" of mojom enum FooEnum from uint32_t to
>>>> FooEnum_Data,
>>>>
>>> in order to enable enum validation. For native only enums, I think it
>>> should
>>> remain uint32_t?
>>>
>>>
>>> SGTM
>>
>>
>> Btw, are you planning to add validation for customized enums or would
>> like me to make the change?
>>
>
> I was considering it, but it's not clear to me what the right way to do
> this is. One simple approach would be to provide a macro which can take an
> arbitrary boolean expression and it defines some traits for you. Or we
> could just have you provide your own validation traits. WDYT?
>

I personally prefer traits (i.e., template magic!) than macros (black
magic), but that is just my personal preference. :)

Where do you think we should insert this logic?
Do we generate Enum_Data just like standard mojom enums and call the
traits/macros in Enum_Data::IsKnonwValue()? Or do it at the deserialization
stage just like customized structs?
The former approach will have mostly the same code as standard mojom enums
and therefore reduce branches in our jinja templates.
The latter will be more consistent with customized structs.



>
>
>>
>>
>>>
>>>
>>>
>>>
>>>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
>>>
>>>> File components/mus/public/interfaces/compositor_frame.mojom (right):
>>>>
>>>
>>>
>>>
>>>
https://codereview.chromium.org/1635613002/diff/20001/components/mus/public/i...
>>>
>>>> components/mus/public/interfaces/compositor_frame.mojom:20:
>>>> [native=True]
>>>> Considering that we have MinVersion (and previously "Client"), I think
>>>> this
>>>>
>>> should probably be "Native". (Sorry I didn't catch this in previous code
>>> reviews.)
>>>
>>>
>>> Yeah I saw your bug and you're right. I'll change that in a new CL before
>>> landing this one.
>>>
>>>
>>>
>>>
>>>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
>>>
>>>> File mandoline/ui/desktop_ui/BUILD.gn (right):
>>>>
>>>
>>>
>>>
>>>
https://codereview.chromium.org/1635613002/diff/100001/mandoline/ui/desktop_u...
>>>
>>>> mandoline/ui/desktop_ui/BUILD.gn:48: "public/interfaces",
>>>> nit: I think according to GN style guide, it is preferred to use a path
>>>>
>>> starting from '//', unless it is from the same file.
>>>
>>>
>>>
>>>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
>>>
>>>> File
>>>> mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
>>>>
>>> (right):
>>>
>>>
>>>
>>>
https://codereview.chromium.org/1635613002/diff/100001/mojo/public/tools/bind...
>>>
>>>> mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:20:
>>>> {%
>>>>
>>> endif %}
>>>
>>>> nit: "{%-" (and also line 54)
>>>>
>>>
>>>
>>> https://codereview.chromium.org/1635613002/
>>>
>>
>>
>

-- 
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.

[Ken Rockot(use gerrit already), 2016-01-31 06:09:49]

Description was changed from

==========
Let Mojo bindings reuse existing enum classes

This lets us declare a native enum in mojom with a
typemap pointing to the existing typename.

To support this a new explicit cpp_only option is
added to mojom rules.

mus::mojom::CommandBufferNamespace is converted to
an alias of gpu::CommandBufferNamespace as a
proof-of-concept, as well as
mus::mojom::ResourceFormat => cc::ResourceFormat

BUG=404186
==========

to

==========
Let Mojo bindings reuse existing enum classes

This lets us declare a native enum in mojom with a
typemap pointing to the existing typename.

To support this a new explicit cpp_only option is
added to mojom rules.

mus::mojom::CommandBufferNamespace is converted to
an alias of gpu::CommandBufferNamespace as a
proof-of-concept, as well as
mus::mojom::ResourceFormat => cc::ResourceFormat

BUG=404186
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ken Rockot(use gerrit already), 2016-01-31 06:11:34]

Description was changed from

==========
Let Mojo bindings reuse existing enum classes

This lets us declare a native enum in mojom with a
typemap pointing to the existing typename.

To support this a new explicit cpp_only option is
added to mojom rules.

mus::mojom::CommandBufferNamespace is converted to
an alias of gpu::CommandBufferNamespace as a
proof-of-concept, as well as
mus::mojom::ResourceFormat => cc::ResourceFormat

BUG=404186
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
[mojo-bindings] Support reuse of native enum classes

This CL supports the Native=True attribute on empty
mojom enum declarations, allowing typemaps to establish
the mojom enum name as an alias for some native enum
class defined elsewhere.

Optional custom validation is supported via
specialization of mojo::EnumTraits<T>.
==========

[Ken Rockot(use gerrit already), 2016-01-31 15:47:43]

Description was changed from

==========
[mojo-bindings] Support reuse of native enum classes

This CL supports the Native=True attribute on empty
mojom enum declarations, allowing typemaps to establish
the mojom enum name as an alias for some native enum
class defined elsewhere.

Optional custom validation is supported via
specialization of mojo::EnumTraits<T>.
==========

to

==========
[mojo-bindings] Support reuse of native enum classes

This CL supports the Native=True attribute on empty
mojom enum declarations, allowing typemaps to establish
the mojom enum name as an alias for some native enum
class defined elsewhere.

Optional custom validation is supported via
specialization of mojo::EnumTraits<T>.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ken Rockot(use gerrit already), 2016-01-31 15:54:29]

Patchset #8 (id:140001) has been deleted

[Ken Rockot(use gerrit already), 2016-01-31 15:55:54]

Patchset #7 (id:120001) has been deleted

[xhwang, 2016-03-29 05:04:39]

xhwang@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2016-03-29 05:04:40]

Just realized that this CL is closed without being committed. Are we giving up
on the idea of reusing native enum classes?

[Ken Rockot(use gerrit already), 2016-03-29 05:10:05]

On 2016/03/29 at 05:04:40, xhwang wrote:
> Just realized that this CL is closed without being committed. Are we giving up
on the idea of reusing native enum classes?

I think I'd like to avoid it now, because it adds complexity without any clear
benefit. I don't see any practical down side to moving enum definitions into
mojom if they need to be used for IPC. It's trivial to define a non-mojom alias
for a mojom enum type if library developers want to hide the mojom detail from
their users.

[xhwang, 2016-03-29 05:38:02]

On 2016/03/29 05:10:05, Ken Rockot wrote:
> On 2016/03/29 at 05:04:40, xhwang wrote:
> > Just realized that this CL is closed without being committed. Are we giving
up
> on the idea of reusing native enum classes?
> 
> I think I'd like to avoid it now, because it adds complexity without any clear
> benefit. I don't see any practical down side to moving enum definitions into
> mojom if they need to be used for IPC. It's trivial to define a non-mojom
alias
> for a mojom enum type if library developers want to hide the mojom detail from
> their users.

sgtm, thanks for the explanation! I can close
https://bugs.chromium.org/p/chromium/issues/detail?id=581405 now :)