IDL: Enumeration support in union types

IDL: Enumeration support in union types

We should be able to use an enumeration type as a union member type.
It is treated as a DOMString, with value checking. A union type container
which has a enum member type validates its value when it is set
(both JS and C++).

BUG=240176
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188543

[bashi, 2015-01-16 09:30:30]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, jl@opera.com

[bashi, 2015-01-16 09:30:31]

PTAL?

Example use of this feature(from Fredrik Söderquist):
http://dev.w3.org/html5/webvtt/#the-vttcue-interface

[Jens Widell, 2015-01-16 09:53:51]

https://codereview.chromium.org/854113002/diff/1/Source/bindings/scripts/v8_u...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/854113002/diff/1/Source/bindings/scripts/v8_u...
Source/bindings/scripts/v8_union.py:99: elif member.is_enum:
If there's an enum type in the union, it will be union_type.string_member_type,
and reading string_member_type would throw an error (about ambiguity) if there's
both an enum and some other string.

So the additions here in this loop aren't really needed; we could could later
check if string_type.is_enum is true.

In fact, I think all the added handling of enumerations in this function is
unneeded; handling the enum validation expression in member_context() would be
enough.

https://codereview.chromium.org/854113002/diff/1/Source/bindings/templates/un...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/854113002/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:138: {# 16.1 Enum #}
If we treat an enum as a string type, this block could be put inside the generic
string case, as

  {% if container.string_type.enum_validation_expression %}
  String string = cppValue;
  ...
  {% endif %}

[bashi, 2015-01-16 10:11:58]

Thank you for review!

https://codereview.chromium.org/854113002/diff/1/Source/bindings/scripts/v8_u...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/854113002/diff/1/Source/bindings/scripts/v8_u...
Source/bindings/scripts/v8_union.py:99: elif member.is_enum:
On 2015/01/16 09:53:51, Jens Widell wrote:
> If there's an enum type in the union, it will be
union_type.string_member_type,
> and reading string_member_type would throw an error (about ambiguity) if
there's
> both an enum and some other string.

My understanding is that enums and strings are not distinguishable and that's
why I added exceptions in line 101 and 109. However, it seems that the spec
doesn't explicitly say that, so maybe we need to file a bug for the spec. 
http://heycam.github.io/webidl/#dfn-distinguishable

> 
> So the additions here in this loop aren't really needed; we could could later
> check if string_type.is_enum is true.
> 
> In fact, I think all the added handling of enumerations in this function is
> unneeded; handling the enum validation expression in member_context() would be
> enough.

Hmm, does it mean we will provide getAsString() getter instead of getFooEnum()
for "enum FooEnum { ... }"? I'd prefer to provide the later.

https://codereview.chromium.org/854113002/diff/1/Source/bindings/templates/un...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/854113002/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:138: {# 16.1 Enum #}
On 2015/01/16 09:53:51, Jens Widell wrote:
> If we treat an enum as a string type, this block could be put inside the
generic
> string case, as
> 
>   {% if container.string_type.enum_validation_expression %}
>   String string = cppValue;
>   ...
>   {% endif %}

Right. Will do when we decide to treat an enum as a string.

[bashi, 2015-01-16 10:19:46]

On 2015/01/16 10:11:58, bashi1 wrote:
> Thank you for review!
> 
>
https://codereview.chromium.org/854113002/diff/1/Source/bindings/scripts/v8_u...
> File Source/bindings/scripts/v8_union.py (right):
> 
>
https://codereview.chromium.org/854113002/diff/1/Source/bindings/scripts/v8_u...
> Source/bindings/scripts/v8_union.py:99: elif member.is_enum:
> On 2015/01/16 09:53:51, Jens Widell wrote:
> > If there's an enum type in the union, it will be
> union_type.string_member_type,
> > and reading string_member_type would throw an error (about ambiguity) if
> there's
> > both an enum and some other string.
> 
> My understanding is that enums and strings are not distinguishable and that's
> why I added exceptions in line 101 and 109. However, it seems that the spec
> doesn't explicitly say that, so maybe we need to file a bug for the spec. 
> http://heycam.github.io/webidl/#dfn-distinguishable
> 
> > 
> > So the additions here in this loop aren't really needed; we could could
later
> > check if string_type.is_enum is true.
> > 
> > In fact, I think all the added handling of enumerations in this function is
> > unneeded; handling the enum validation expression in member_context() would
be
> > enough.
> 
> Hmm, does it mean we will provide getAsString() getter instead of getFooEnum()
> for "enum FooEnum { ... }"? I'd prefer to provide the later.
> 
>
https://codereview.chromium.org/854113002/diff/1/Source/bindings/templates/un...
> File Source/bindings/templates/union.cpp (right):
> 
>
https://codereview.chromium.org/854113002/diff/1/Source/bindings/templates/un...
> Source/bindings/templates/union.cpp:138: {# 16.1 Enum #}
> On 2015/01/16 09:53:51, Jens Widell wrote:
> > If we treat an enum as a string type, this block could be put inside the
> generic
> > string case, as
> > 
> >   {% if container.string_type.enum_validation_expression %}
> >   String string = cppValue;
> >   ...
> >   {% endif %}
> 
> Right. Will do when we decide to treat an enum as a string.

Oh, I misunderstood your suggestion. Your suggestion is much better. Will revise
the CL shortly.

[bashi, 2015-01-16 10:23:39]

Revised the CL. Could you take another look?

[Jens Widell, 2015-01-16 10:39:33]

LGTM!

[bashi, 2015-01-16 11:03:44]

Thanks!

[bashi, 2015-01-16 11:03:50]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-01-16 11:05:00]

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

[commit-bot: I haz the power, 2015-01-16 11:11:27]

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

[commit-bot: I haz the power, 2015-01-16 11:11:28]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[haraken, 2015-01-16 13:04:15]

LGTM

[bashi, 2015-01-16 16:12:31]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-01-16 16:13:21]

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

[commit-bot: I haz the power, 2015-01-16 21:51:30]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188543