IDL union: nullable support

IDL union: nullable support

Creating container class for nullable union types would generate a lot of
duplications. Suppose we have (A or B) and (A or B)?. The current
implementation will generate two classes: AOrB and AOrBOrNull.
The only difference between them is whether toImpl() checks
isUndefinedOrNull() at first. To avoid duplication, this CL generates
toImpl() only for nullable union types.

This CL includes part of Jens' work[1]

[1] https://codereview.chromium.org/700093002/

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

[bashi, 2014-11-05 08:23:45]

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

[bashi, 2014-11-05 08:23:45]

Haraken-san, Jens,

I'm not asking for actual review, but want to hear your opinions. Could you take
a look?

[haraken, 2014-11-05 08:32:53]

I'd prefer the approach 2 or 3, since (A or B) and (A or B)? are different
types.

I agree that the approach 2 will increase generated code (because we need to
generate a container class for AOrBOrNull), but does the approach 3 increase
generated code as well? I guess we can share a container class for AOrB and just
need a couple of tricks to make Nullable<AOrB> workable.

[Jens Widell, 2014-11-05 08:45:06]

I'd propose we do something like this: https://codereview.chromium.org/700093002

[bashi, 2014-11-05 09:20:51]

Thank you for the inputs, Haraken-san, Jens!

> I agree that the approach 2 will increase generated code (because we need to
> generate a container class for AOrBOrNull), but does the approach 3 increase
> generated code as well?

Yes. I guess increasing rate would be smaller than 2, though.

On 2014/11/05 08:45:06, Jens Widell wrote:
> I'd propose we do something like this:
https://codereview.chromium.org/700093002

Oh, I should have waited for this CL...

Let me try to do approach 2 or 3, as Haraken-san said that (A or B) and (A or
B)? is a different type(for record, Shiino-san said the same thing when we have
a discussion) and we should treat them separately if there is no performance
issue.

I'll work on this on Friday.

[Yuki, 2014-11-05 09:29:18]

On 2014/11/05 09:20:51, bashi1 wrote:
> Thank you for the inputs, Haraken-san, Jens!
> 
> > I agree that the approach 2 will increase generated code (because we need to
> > generate a container class for AOrBOrNull), but does the approach 3 increase
> > generated code as well?
> 
> Yes. I guess increasing rate would be smaller than 2, though.
> 
> On 2014/11/05 08:45:06, Jens Widell wrote:
> > I'd propose we do something like this:
> https://codereview.chromium.org/700093002
> 
> Oh, I should have waited for this CL...
> 
> Let me try to do approach 2 or 3, as Haraken-san said that (A or B) and (A or
> B)? is a different type(for record, Shiino-san said the same thing when we
have
> a discussion) and we should treat them separately if there is no performance
> issue.
> 
> I'll work on this on Friday.

Option 3: Nullable<AOrB> seems better than option 2, because the customers
may want to convert AOrBOrNull to AOrB after checking that it's not null.

For example,

    void helper(AOrB impl) { ... }

    void method(Nullable<AOrB> impl) {
        if (impl.isNull) {
            ...
        } else {
            helper(impl.get());
        }
    }

I'm not quite sure how often the customers would like to convert, but it
may be useful.

[haraken, 2014-11-05 09:30:14]

BTW, I gave a short talk about IDL union type at a lightning talk of BlinkOn3. I
received a lot of positive feedback and realized that people are really waiting
for union types :)

[bashi, 2014-11-05 09:34:29]

On 2014/11/05 09:30:14, haraken wrote:
> BTW, I gave a short talk about IDL union type at a lightning talk of BlinkOn3.
I
> received a lot of positive feedback and realized that people are really
waiting
> for union types :)

Glad to hear that:)

[Jens Widell, 2014-11-05 09:46:37]

On 2014/11/05 09:29:18, Yuki wrote:
> On 2014/11/05 09:20:51, bashi1 wrote:
> > Thank you for the inputs, Haraken-san, Jens!
> > 
> > > I agree that the approach 2 will increase generated code (because we need
to
> > > generate a container class for AOrBOrNull), but does the approach 3
increase
> > > generated code as well?
> > 
> > Yes. I guess increasing rate would be smaller than 2, though.
> > 
> > On 2014/11/05 08:45:06, Jens Widell wrote:
> > > I'd propose we do something like this:
> > https://codereview.chromium.org/700093002
> > 
> > Oh, I should have waited for this CL...
> > 
> > Let me try to do approach 2 or 3, as Haraken-san said that (A or B) and (A
or
> > B)? is a different type(for record, Shiino-san said the same thing when we
> have
> > a discussion) and we should treat them separately if there is no performance
> > issue.
> > 
> > I'll work on this on Friday.
> 
> Option 3: Nullable<AOrB> seems better than option 2, because the customers
> may want to convert AOrBOrNull to AOrB after checking that it's not null.
> 
> For example,
> 
>     void helper(AOrB impl) { ... }
> 
>     void method(Nullable<AOrB> impl) {
>         if (impl.isNull) {
>             ...
>         } else {
>             helper(impl.get());
>         }
>     }
> 
> I'm not quite sure how often the customers would like to convert, but it
> may be useful.

I think we should generally only use Nullable<> for types we're not in control
of and that can't represent a null value. For types we design ourselves
(dictionaries, union containers, strings) we should just make room for a null
value representation and use that.

Also, for out/result parameters, wrapping with Nullable<> is a bit strange. It
basically requires by-value semantics for the wrapped value, and if we wanted
that we might as well not use an out/result parameter at all, and just return
union containers by value.

[bashi, 2014-11-07 00:03:17]

On 2014/11/05 09:46:37, Jens Widell wrote:
> On 2014/11/05 09:29:18, Yuki wrote:
> > On 2014/11/05 09:20:51, bashi1 wrote:
> > > Thank you for the inputs, Haraken-san, Jens!
> > > 
> > > > I agree that the approach 2 will increase generated code (because we
need
> to
> > > > generate a container class for AOrBOrNull), but does the approach 3
> increase
> > > > generated code as well?
> > > 
> > > Yes. I guess increasing rate would be smaller than 2, though.
> > > 
> > > On 2014/11/05 08:45:06, Jens Widell wrote:
> > > > I'd propose we do something like this:
> > > https://codereview.chromium.org/700093002
> > > 
> > > Oh, I should have waited for this CL...
> > > 
> > > Let me try to do approach 2 or 3, as Haraken-san said that (A or B) and (A
> or
> > > B)? is a different type(for record, Shiino-san said the same thing when we
> > have
> > > a discussion) and we should treat them separately if there is no
performance
> > > issue.
> > > 
> > > I'll work on this on Friday.
> > 
> > Option 3: Nullable<AOrB> seems better than option 2, because the customers
> > may want to convert AOrBOrNull to AOrB after checking that it's not null.
> > 
> > For example,
> > 
> >     void helper(AOrB impl) { ... }
> > 
> >     void method(Nullable<AOrB> impl) {
> >         if (impl.isNull) {
> >             ...
> >         } else {
> >             helper(impl.get());
> >         }
> >     }
> > 
> > I'm not quite sure how often the customers would like to convert, but it
> > may be useful.
> 
> I think we should generally only use Nullable<> for types we're not in control
> of and that can't represent a null value. For types we design ourselves
> (dictionaries, union containers, strings) we should just make room for a null
> value representation and use that.
> 
> Also, for out/result parameters, wrapping with Nullable<> is a bit strange. It
> basically requires by-value semantics for the wrapped value, and if we wanted
> that we might as well not use an out/result parameter at all, and just return
> union containers by value.

Hmm, that also makes sense. We don't handle nullable separately for strings and
wrappers.

Given following IDL methods,
  void method1(Element? elem);                                                  
                                                  
  Element? method2();                                                           
                                                  
  void method3(String? str);                                                    
                                                  
  String? method4();                                                            
                                                  

The impl class will have following methods:
  void method1(RefPtr<Element> elem);                                           
                                                  
  RefPtr<Element> method2();                                                    
                                                  
  void method3(String str);                                                     
                                                  
  String method4();                                                             
                                                  

If we think union containers this way, option 1 wouldn't sound strange to Blink
developers (they should be familiar with how strin\
gs and wrappers handle null).

I think we can take option 1 first, then revisit if it looks weird. Haraken-san,
Shiino-san, WDYT?

[haraken, 2014-11-07 00:08:29]

On 2014/11/07 00:03:17, bashi1 wrote:
> On 2014/11/05 09:46:37, Jens Widell wrote:
> > On 2014/11/05 09:29:18, Yuki wrote:
> > > On 2014/11/05 09:20:51, bashi1 wrote:
> > > > Thank you for the inputs, Haraken-san, Jens!
> > > > 
> > > > > I agree that the approach 2 will increase generated code (because we
> need
> > to
> > > > > generate a container class for AOrBOrNull), but does the approach 3
> > increase
> > > > > generated code as well?
> > > > 
> > > > Yes. I guess increasing rate would be smaller than 2, though.
> > > > 
> > > > On 2014/11/05 08:45:06, Jens Widell wrote:
> > > > > I'd propose we do something like this:
> > > > https://codereview.chromium.org/700093002
> > > > 
> > > > Oh, I should have waited for this CL...
> > > > 
> > > > Let me try to do approach 2 or 3, as Haraken-san said that (A or B) and
(A
> > or
> > > > B)? is a different type(for record, Shiino-san said the same thing when
we
> > > have
> > > > a discussion) and we should treat them separately if there is no
> performance
> > > > issue.
> > > > 
> > > > I'll work on this on Friday.
> > > 
> > > Option 3: Nullable<AOrB> seems better than option 2, because the customers
> > > may want to convert AOrBOrNull to AOrB after checking that it's not null.
> > > 
> > > For example,
> > > 
> > >     void helper(AOrB impl) { ... }
> > > 
> > >     void method(Nullable<AOrB> impl) {
> > >         if (impl.isNull) {
> > >             ...
> > >         } else {
> > >             helper(impl.get());
> > >         }
> > >     }
> > > 
> > > I'm not quite sure how often the customers would like to convert, but it
> > > may be useful.
> > 
> > I think we should generally only use Nullable<> for types we're not in
control
> > of and that can't represent a null value. For types we design ourselves
> > (dictionaries, union containers, strings) we should just make room for a
null
> > value representation and use that.
> > 
> > Also, for out/result parameters, wrapping with Nullable<> is a bit strange.
It
> > basically requires by-value semantics for the wrapped value, and if we
wanted
> > that we might as well not use an out/result parameter at all, and just
return
> > union containers by value.
> 
> Hmm, that also makes sense. We don't handle nullable separately for strings
and
> wrappers.
> 
> Given following IDL methods,
>   void method1(Element? elem);                                                
 
>                                                   
>   Element? method2();                                                         
 
>                                                   
>   void method3(String? str);                                                  
 
>                                                   
>   String? method4();                                                          
 
>                                                   
> 
> The impl class will have following methods:
>   void method1(RefPtr<Element> elem);                                         
 
>                                                   
>   RefPtr<Element> method2();                                                  
 
>                                                   
>   void method3(String str);                                                   
 
>                                                   
>   String method4();                                                           
 
>                                                   
> 
> If we think union containers this way, option 1 wouldn't sound strange to
Blink
> developers (they should be familiar with how strin\
> gs and wrappers handle null).
> 
> I think we can take option 1 first, then revisit if it looks weird.
Haraken-san,
> Shiino-san, WDYT?

Jens' point makes sense. I agree with the option 1.

[bashi, 2014-11-07 03:53:00]

Revised the CL for actual review. Could you take a look at the CL?

I'll add a layout test in a follow-up CL if this looks reasonable.
https://codereview.chromium.org/692993003/

(We don't collect union types from Source/core/testing so we can't use union
types which are only used in tests)

[Jens Widell, 2014-11-07 07:11:53]

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:196: return idl_type.as_union_type.name
So, if you have (A? or B), the name generated here will be "AOrNullOrB", and
then we'll have V8AOrNullOrB::toImpl() which doesn't handle null and
V8AOrNullOrBOrNull::toImpl() which does. :-)

This type name generation is per WebIDL, but for the purpose of generating C++
classes internally it would make sense to strip nullability from the members and
treat it separately (as the rest of this CL does.)

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestObject.idl:160: attribute (double or
DOMString)? doubleOrStringOrNullAttribute;
Add a (double? or DOMString) attribute as well?

[bashi, 2014-11-07 10:07:53]

Thank you for review!

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:196: return idl_type.as_union_type.name
On 2014/11/07 07:11:53, Jens Widell wrote:
> So, if you have (A? or B), the name generated here will be "AOrNullOrB", and
> then we'll have V8AOrNullOrB::toImpl() which doesn't handle null and
> V8AOrNullOrBOrNull::toImpl() which does. :-)
> 
> This type name generation is per WebIDL, but for the purpose of generating C++
> classes internally it would make sense to strip nullability from the members
and
> treat it separately (as the rest of this CL does.)

You are right. The cpp_type is not necessary the same as Web IDL's type name,
and I think we can treat (A? or B) and (A or B)? in the same way (I can't think
of a situation that we want to distinguish them). Done.

For record, it seems that (A? or B)? is allowed (I'm not 100% sure), but I also
think we can handle this the same way(generate AOrB and V8AOrBOrNull::toImpl).

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestObject.idl:160: attribute (double or
DOMString)? doubleOrStringOrNullAttribute;
On 2014/11/07 07:11:53, Jens Widell wrote:
> Add a (double? or DOMString) attribute as well?

Done.

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

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/template...
Source/bindings/templates/union.h:69: class V8{{cpp_type}}OrNull final {
Maybe "IncludesNull" is accurate, but I'm not sure it sounds fine.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestObject.idl:361: void
voidMethodDoubleOrDOMString((double or DOMString) arg);
These method name were wrong (my bad).

[Jens Widell, 2014-11-07 10:58:19]

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:196: return idl_type.as_union_type.name
On 2014/11/07 10:07:53, bashi1 wrote:
> On 2014/11/07 07:11:53, Jens Widell wrote:
> > So, if you have (A? or B), the name generated here will be "AOrNullOrB", and
> > then we'll have V8AOrNullOrB::toImpl() which doesn't handle null and
> > V8AOrNullOrBOrNull::toImpl() which does. :-)
> > 
> > This type name generation is per WebIDL, but for the purpose of generating
C++
> > classes internally it would make sense to strip nullability from the members
> and
> > treat it separately (as the rest of this CL does.)
> 
> You are right. The cpp_type is not necessary the same as Web IDL's type name,
> and I think we can treat (A? or B) and (A or B)? in the same way (I can't
think
> of a situation that we want to distinguish them). Done.

The types (A? or B) and (A or B)? should be handled the same in all ways,
AFAICT.

> For record, it seems that (A? or B)? is allowed (I'm not 100% sure), but I
also
> think we can handle this the same way(generate AOrB and V8AOrBOrNull::toImpl).

It's probably allowed, but should also be handled the same way. One could
imagine that happening (by mistake) in a spec if the union is a typedef:

  typedef (A? or B) Thing;

and then

  void f(Thing? x);

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:202: member_names =
"Or".join([member_cpp_name(member)
Drop the [] to make it a generator expression instead of a list comprehension?
And return the result directly instead of storing it in a temporary local?

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:31: if union_type.includes_nullable_type:
Should this also check union_type.is_nullable?

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:35: key=lambda union_type: union_type.name)
It doesn't really matter (stable order in the generated code is the priority)
but with (A? or B) and (A or C) you'll get the order AOrC, AorB in the generated
code, won't you?

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

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/template...
Source/bindings/templates/union.h:69: class V8{{cpp_type}}OrNull final {
On 2014/11/07 10:07:53, bashi1 wrote:
> Maybe "IncludesNull" is accurate, but I'm not sure it sounds fine.

"AOrBCommaOrNull" (for "A or B, or null")? ;-)

If we want to avoid the (apparent) ambiguity of whether AOrBOrNull is (A or B?)
or (A or B)?, we could also make it NullOrAOrB.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestObject.idl:361: void
voidMethodDoubleOrDOMString((double or DOMString) arg);
On 2014/11/07 10:07:53, bashi1 wrote:
> These method name were wrong (my bad).

Technically they still are. :-)

Should be "voidMethodDoubleOrDOMStringArg" according to the naming convention,
which is documented above in this file.

[haraken, 2014-11-07 18:06:32]

Looks good.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:197: # V8AOrBOrNull to handle nulle for (A?
or B), as well as (A or B)?

to handle nulle for ... => for (A? or B), (A or B?) and (A or B)?.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:29:
union_types_for_containers.add(union_type.as_union_type)

I'm not sure what .as_union_type is for. Can we just use union_type ?

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:105: 'includes_nullable_type':
union_type.includes_nullable_type,

includes_nullable_type => contains_nullable_type ?

Or simply, is_nullable ?

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

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/template...
Source/bindings/templates/union.h:69: class V8{{cpp_type}}OrNull final {
On 2014/11/07 10:58:19, Jens Widell wrote:
> On 2014/11/07 10:07:53, bashi1 wrote:
> > Maybe "IncludesNull" is accurate, but I'm not sure it sounds fine.
> 
> "AOrBCommaOrNull" (for "A or B, or null")? ;-)
> 
> If we want to avoid the (apparent) ambiguity of whether AOrBOrNull is (A or
B?)
> or (A or B)?, we could also make it NullOrAOrB.

I'm OK with AOrBOrNull :)

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/template...
Source/bindings/templates/union.h:74: {# 1. null or undefined #}

Can we add unit tests for the following cases?

// IDL
void foo((A or B)? arg);
void bar((A or B) arg);

// JS
foo(null);
foo(undefined);
bar(null);
bar(undefined);

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/UnionTypesCore.h (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:413: 

Unnecessary empty line.

[Jens Widell, 2014-11-07 18:15:46]

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:29:
union_types_for_containers.add(union_type.as_union_type)
On 2014/11/07 18:06:31, haraken wrote:
> 
> I'm not sure what .as_union_type is for. Can we just use union_type ?

Since container_context() uses union_type.name, it makes a difference. But it
only affects (Python) exception messages, so the difference is insignificant.

[bashi, 2014-11-08 06:59:10]

Patchset #4 (id:60001) has been deleted

[bashi, 2014-11-08 07:10:35]

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/698423002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:196: return idl_type.as_union_type.name
On 2014/11/07 10:58:19, Jens Widell wrote:
> On 2014/11/07 10:07:53, bashi1 wrote:
> > On 2014/11/07 07:11:53, Jens Widell wrote:
> > > So, if you have (A? or B), the name generated here will be "AOrNullOrB",
and
> > > then we'll have V8AOrNullOrB::toImpl() which doesn't handle null and
> > > V8AOrNullOrBOrNull::toImpl() which does. :-)
> > > 
> > > This type name generation is per WebIDL, but for the purpose of generating
> C++
> > > classes internally it would make sense to strip nullability from the
members
> > and
> > > treat it separately (as the rest of this CL does.)
> > 
> > You are right. The cpp_type is not necessary the same as Web IDL's type
name,
> > and I think we can treat (A? or B) and (A or B)? in the same way (I can't
> think
> > of a situation that we want to distinguish them). Done.
> 
> The types (A? or B) and (A or B)? should be handled the same in all ways,
> AFAICT.
> 
> > For record, it seems that (A? or B)? is allowed (I'm not 100% sure), but I
> also
> > think we can handle this the same way(generate AOrB and
V8AOrBOrNull::toImpl).
> 
> It's probably allowed, but should also be handled the same way. One could
> imagine that happening (by mistake) in a spec if the union is a typedef:
> 
>   typedef (A? or B) Thing;
> 
> and then
> 
>   void f(Thing? x);

Thanks for the confirmation:)

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:197: # V8AOrBOrNull to handle nulle for (A?
or B), as well as (A or B)?
On 2014/11/07 18:06:31, haraken wrote:
> 
> to handle nulle for ... => for (A? or B), (A or B?) and (A or B)?.

Done.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:202: member_names =
"Or".join([member_cpp_name(member)
On 2014/11/07 10:58:19, Jens Widell wrote:
> Drop the [] to make it a generator expression instead of a list comprehension?
> And return the result directly instead of storing it in a temporary local?

Done.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:29:
union_types_for_containers.add(union_type.as_union_type)
On 2014/11/07 18:15:46, Jens Widell wrote:
> On 2014/11/07 18:06:31, haraken wrote:
> > 
> > I'm not sure what .as_union_type is for. Can we just use union_type ?
> 
> Since container_context() uses union_type.name, it makes a difference. But it
> only affects (Python) exception messages, so the difference is insignificant.

Yes, it only affects exception messages, but just using union_type would be
suitable. Done.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:31: if union_type.includes_nullable_type:
On 2014/11/07 10:58:19, Jens Widell wrote:
> Should this also check union_type.is_nullable?

IdlNullableType.includes_nullable_type is True so it should already be covered.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:35: key=lambda union_type: union_type.name)
On 2014/11/07 10:58:19, Jens Widell wrote:
> It doesn't really matter (stable order in the generated code is the priority)
> but with (A? or B) and (A or C) you'll get the order AOrC, AorB in the
generated
> code, won't you?

Good catch! Then, we should use idl_type.cpp_type as the sort key. Done.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:105: 'includes_nullable_type':
union_type.includes_nullable_type,
On 2014/11/07 18:06:31, haraken wrote:
> 
> includes_nullable_type => contains_nullable_type ?
> 
> Or simply, is_nullable ?

"includes a nullable type" is what the spec is using so I'd prefer not changing
the name.
http://heycam.github.io/webidl/#dfn-includes-a-nullable-type

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

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/template...
Source/bindings/templates/union.h:69: class V8{{cpp_type}}OrNull final {
On 2014/11/07 18:06:31, haraken wrote:
> On 2014/11/07 10:58:19, Jens Widell wrote:
> > On 2014/11/07 10:07:53, bashi1 wrote:
> > > Maybe "IncludesNull" is accurate, but I'm not sure it sounds fine.
> > 
> > "AOrBCommaOrNull" (for "A or B, or null")? ;-)
> > 
> > If we want to avoid the (apparent) ambiguity of whether AOrBOrNull is (A or
> B?)
> > or (A or B)?, we could also make it NullOrAOrB.
> 
> I'm OK with AOrBOrNull :)

Let's use "OrNull" suffix :)

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/template...
Source/bindings/templates/union.h:74: {# 1. null or undefined #}
On 2014/11/07 18:06:31, haraken wrote:
> 
> Can we add unit tests for the following cases?
> 
> // IDL
> void foo((A or B)? arg);
> void bar((A or B) arg);
> 
> // JS
> foo(null);
> foo(undefined);
> bar(null);
> bar(undefined);

Yes, that's what I'm going to add in a follow-up CL.
https://codereview.chromium.org/692993003/

The reason why I didn't include them is that we can only use union types which
are already defined in non-test IDLs. To add layout tests, we need to modify
existing IDL files (HTMLAllCollection in this case). I don't want to includes
such changes in this CL.

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/core/TestObject.idl:361: void
voidMethodDoubleOrDOMString((double or DOMString) arg);
On 2014/11/07 10:58:19, Jens Widell wrote:
> On 2014/11/07 10:07:53, bashi1 wrote:
> > These method name were wrong (my bad).
> 
> Technically they still are. :-)
> 
> Should be "voidMethodDoubleOrDOMStringArg" according to the naming convention,
> which is documented above in this file.

Oops... Done:)

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/UnionTypesCore.h (right):

https://codereview.chromium.org/698423002/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:413: 
On 2014/11/07 18:06:32, haraken wrote:
> 
> Unnecessary empty line.

Done.

[haraken, 2014-11-08 08:27:24]

LGTM

[Jens Widell, 2014-11-08 08:34:34]

LGTM

[bashi, 2014-11-08 08:57:43]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-11-08 08:59:06]

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

[commit-bot: I haz the power, 2014-11-08 11:14:46]

Committed patchset #4 (id:80001) as 185005