IDL: Union type support for attributes

IDL: Union type support for attributes

Some specs use union type attributes. For example, TrackEvent[1]
has 'track' attribute, of which type is
(VideoTrack or AudioTrack or TextTrack). Let's support union type
attributes so that we can reduce custom bindings. Follow-up CLs
will replace custom bindings if possible.

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

[bashi, 2014-11-04 08:43:53]

Patchset #1 (id:1) has been deleted

[bashi, 2014-11-04 09:52:52]

Patchset #1 (id:20001) has been deleted

[bashi, 2014-11-04 11:32:48]

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

[bashi, 2014-11-04 11:32:49]

Hi, could you review this CL?

https://codereview.chromium.org/704503002/diff/40001/Source/bindings/template...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/704503002/diff/40001/Source/bindings/template...
Source/bindings/templates/union.h:29: {{member.rvalue_cpp_type}}
getAs{{member.type_name}}() const;
Need "const" for argument setters like below:

void setFoo(const DoubleOrString& doubleOrString)
{
    if (doubleOrString.isDouble()) {
        double d = doubleOrString.getAsDouble();
        ...
    }
    ...
}

[Jens Widell, 2014-11-04 12:19:07]

LGTM

https://codereview.chromium.org/704503002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/704503002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:240: if
attribute.idl_type.is_union_type:
Use v8_methods.use_output_parameter_for_result() perhaps? And if so maybe move
it to v8_types first, as an IdlType attribute, so we can have

  if attribute.idl_type.use_output_parameter_for_result:
    arguments.append('result')

I'm assuming that if we want to support dictionary type attributes, this is how
we'd do it.

[haraken, 2014-11-04 23:50:09]

LGTM

https://codereview.chromium.org/704503002/diff/60001/LayoutTests/fast/dom/idl...
File LayoutTests/fast/dom/idl-union-type-unittest.html (right):

https://codereview.chromium.org/704503002/diff/60001/LayoutTests/fast/dom/idl...
LayoutTests/fast/dom/idl-union-type-unittest.html:19:
shouldBeEqualToString('unionTypesTest.doubleOrStringAttribute', 'undefined');

Nit: shouldBeUndefined

https://codereview.chromium.org/704503002/diff/60001/LayoutTests/fast/dom/idl...
LayoutTests/fast/dom/idl-union-type-unittest.html:21:
shouldBeEqualToString('unionTypesTest.doubleOrStringAttribute', 'null');

Nit: shouldBeNull

https://codereview.chromium.org/704503002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/704503002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:128: 'is_union_type':
idl_type.is_union_type,

is_union_type => use_output_parameter_for_result ?

[bashi, 2014-11-05 00:15:19]

Patchset #3 (id:80001) has been deleted

[bashi, 2014-11-05 00:17:33]

Thank you for reviews!

https://codereview.chromium.org/704503002/diff/60001/LayoutTests/fast/dom/idl...
File LayoutTests/fast/dom/idl-union-type-unittest.html (right):

https://codereview.chromium.org/704503002/diff/60001/LayoutTests/fast/dom/idl...
LayoutTests/fast/dom/idl-union-type-unittest.html:13:
shouldBeNull('unionTypesTest.doubleOrStringAttribute');
I noticed this is wrong. This attribute is non-nullable so this shouldn't become
null. Changed the default value to return zero.

https://codereview.chromium.org/704503002/diff/60001/LayoutTests/fast/dom/idl...
LayoutTests/fast/dom/idl-union-type-unittest.html:19:
shouldBeEqualToString('unionTypesTest.doubleOrStringAttribute', 'undefined');
On 2014/11/04 23:50:09, haraken wrote:
> 
> Nit: shouldBeUndefined

The expected result is "undefined" (of type string), so I think it should be
shouldBeEqualToString.

https://codereview.chromium.org/704503002/diff/60001/LayoutTests/fast/dom/idl...
LayoutTests/fast/dom/idl-union-type-unittest.html:21:
shouldBeEqualToString('unionTypesTest.doubleOrStringAttribute', 'null');
On 2014/11/04 23:50:09, haraken wrote:
> 
> Nit: shouldBeNull

Same as above.

https://codereview.chromium.org/704503002/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_attributes.py (right):

https://codereview.chromium.org/704503002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:128: 'is_union_type':
idl_type.is_union_type,
On 2014/11/04 23:50:09, haraken wrote:
> 
> is_union_type => use_output_parameter_for_result ?

Done.

https://codereview.chromium.org/704503002/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_attributes.py:240: if
attribute.idl_type.is_union_type:
On 2014/11/04 12:19:07, Jens Widell wrote:
> Use v8_methods.use_output_parameter_for_result() perhaps? And if so maybe move
> it to v8_types first, as an IdlType attribute, so we can have
> 
>   if attribute.idl_type.use_output_parameter_for_result:
>     arguments.append('result')
> 
> I'm assuming that if we want to support dictionary type attributes, this is
how
> we'd do it.

Yes. We should use use_output_parameter_for_result. Done.

[bashi, 2014-11-05 00:56:59]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:57:59]

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

[commit-bot: I haz the power, 2014-11-05 01:40:24]

Committed patchset #3 (id:100001) as 184847