IDL: Generate union type containers

IDL: Generate union type containers

Commit b8e05fd added a half baked code generator for union type
containers. This CL implements the code generator and uses them for
union return values.

If (A or B) is given, the name of the corresponding container
type will be "AOrB".

Generated union type containers will have following methods:
- isNull(), true when no specific type.
- isT(), true when the container holds T.
- getAsT(), retrieve the value as T. Need to check isT()
  before calling this.
- setT(), set a value to the container. Allowed only once.

When a method/getter returns an union type, the binding layer
allocates an instance of the container class on stack, then
passes it to impl side as the last argument. The impl method
can set a specific value via setT() methods.
 
We already have build steps for IDL dictionary impl class creation, so
we can use the step for union type containers creation as well. The
code generator takes individual interfaces info, which contains
collected union types, then generates container classes from the
information.

This CL doesn't change the web-faced behavior.

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

[bashi, 2014-10-28 10:09:14]

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

[bashi, 2014-10-28 10:09:15]

Could you take a look?

https://codereview.chromium.org/680193003/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/UnionTypesCore.cpp:70: 
V8 -> impl conversion for sequences and arrays are not implemented yet.
Follow-up CLs will implement them.

[Jens Widell, 2014-10-28 11:31:38]

Looks mostly very good, but some comments.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/core/v8/gene...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/core/v8/gene...
Source/bindings/core/v8/generated.gyp:151:
'<(bindings_core_output_dir)/InterfacesInfoCoreIndividual.pickle',
Add a dependency on 'interfaces_info_individual_core' which generates this file?

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/comp...
File Source/bindings/scripts/compute_interfaces_info_individual.py (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/comp...
Source/bindings/scripts/compute_interfaces_info_individual.py:223:
'has_union_types': len(this_union_types) > 0,  # explicit len(...) > 0 to
convert boolean
Another way of explicitly converting to bool would be "bool(this_union_types)".
A bit better, I think; calculating the length of something for comparison to
zero is a classic anti-pattern.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:402: """Returns a context of return value
for methods/getters of which type
I think "a context" here is expected to be a dict, not a list. This method
returns a list of contexts.

So like in the old code, it might be better to call this method
union_result_members() (and the inner/helper method
union_result_member_context(), as now.)

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:410: cpp_value = '%s.get()' % cpp_value
Could be

  cpp_value += '.get()'

too, which is easier on the eyes, I think. (I also often use string formatting
in lots of situations where simple concatenation would probably be better.)

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:153: if idl_type.is_union_type:
This ought to be moved to after the array/sequence handling. Having it here
breaks arrays or sequences of unions.

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

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_u...
Source/bindings/scripts/v8_union.py:38: platform_objects = []
"interface_types"?

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:281: {% macro
union_type_method_call_and_set_return_value(method) %}
Could the scope of this macro now be reduced to just dealing with the return
value after the call? We already have generic handling of declaring a 'result'
local and passing it as an argument in the call above.

Or, better yet (perhaps), could we simply generate a per-union-type
v8SetReturnValue() method, and completely (?) eliminate the need for any special
code for union return types?

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

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:39: if (v8Value.IsEmpty() ||
isUndefinedOrNull(v8Value))
Null or undefined isn't treated this way in the spec. A null or undefined value
should map to null if there's a nullable member type and otherwise be converted
into a dictionary type if there's such a member type, and otherwise throw a type
error.

If we don't want to follow the spec to the letter on that, then add a FIXME?

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:48: {{platform_object.cpp_type}} cppValue =
V8{{platform_object.type_name}}::toImplWithTypeCheck(isolate, v8Value);
No need to use *WithTypeCheck() here; you just type-checked it. Use

  V8{{platform_object.cpp_type}} cppValue =
V8{{platform_object.cpp_type}}::toImpl(v8::Handle<v8::Object>::Cast(v8_value));

instead.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:56: if (v8Value->IsObject()) {
Spec says "object but not Date or RegExp". If we don't want to follow the spec
ehre for some reason, at least add a (FIXME) comment about the difference.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:74: if (v8Value->IsNumber()) {
You never convert to number (or boolean). If there's no string type, but there
is a number (or boolean) type, you should convert to number (or boolean) at the
end, rather than throwing a type error.

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

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.h:29: {{member.rvalue_cpp_type}}
getAs{{member.type_name}}();
Might be a good idea to inline these?

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.h:34: unsigned m_specificTypeFlag;
For bonus points (nice when debugging) you could use a generated (private)
enumeration type here, I guess.

[bashi, 2014-10-29 01:34:05]

Patchset #2 (id:20001) has been deleted

[bashi, 2014-10-29 01:34:33]

Thank you for review!

https://codereview.chromium.org/680193003/diff/1/Source/bindings/core/v8/gene...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/core/v8/gene...
Source/bindings/core/v8/generated.gyp:151:
'<(bindings_core_output_dir)/InterfacesInfoCoreIndividual.pickle',
On 2014/10/28 11:31:37, Jens Widell wrote:
> Add a dependency on 'interfaces_info_individual_core' which generates this
file?

Done.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/comp...
File Source/bindings/scripts/compute_interfaces_info_individual.py (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/comp...
Source/bindings/scripts/compute_interfaces_info_individual.py:223:
'has_union_types': len(this_union_types) > 0,  # explicit len(...) > 0 to
convert boolean
On 2014/10/28 11:31:37, Jens Widell wrote:
> Another way of explicitly converting to bool would be
"bool(this_union_types)".
> A bit better, I think; calculating the length of something for comparison to
> zero is a classic anti-pattern.

Definitely better way:) Done.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:402: """Returns a context of return value
for methods/getters of which type
On 2014/10/28 11:31:38, Jens Widell wrote:
> I think "a context" here is expected to be a dict, not a list. This method
> returns a list of contexts.
> 
> So like in the old code, it might be better to call this method
> union_result_members() (and the inner/helper method
> union_result_member_context(), as now.)

Now we no longer need this method :)

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:153: if idl_type.is_union_type:
On 2014/10/28 11:31:38, Jens Widell wrote:
> This ought to be moved to after the array/sequence handling. Having it here
> breaks arrays or sequences of unions.

Done.

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

https://codereview.chromium.org/680193003/diff/1/Source/bindings/scripts/v8_u...
Source/bindings/scripts/v8_union.py:38: platform_objects = []
On 2014/10/28 11:31:38, Jens Widell wrote:
> "interface_types"?

Done.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:281: {% macro
union_type_method_call_and_set_return_value(method) %}
On 2014/10/28 11:31:38, Jens Widell wrote:
> Could the scope of this macro now be reduced to just dealing with the return
> value after the call? We already have generic handling of declaring a 'result'
> local and passing it as an argument in the call above.
> 
> Or, better yet (perhaps), could we simply generate a per-union-type
> v8SetReturnValue() method, and completely (?) eliminate the need for any
special
> code for union return types?

Took the later approach, and removed special code for union types from this
template:) Thank you for the suggestion!

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

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:39: if (v8Value.IsEmpty() ||
isUndefinedOrNull(v8Value))
On 2014/10/28 11:31:38, Jens Widell wrote:
> Null or undefined isn't treated this way in the spec. A null or undefined
value
> should map to null if there's a nullable member type and otherwise be
converted
> into a dictionary type if there's such a member type, and otherwise throw a
type
> error.
> 
> If we don't want to follow the spec to the letter on that, then add a FIXME?

Yes, this is tentative, and I should have added a FIXME comment. Added. I'll
revise this in a follow-up CL.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:48: {{platform_object.cpp_type}} cppValue =
V8{{platform_object.type_name}}::toImplWithTypeCheck(isolate, v8Value);
On 2014/10/28 11:31:38, Jens Widell wrote:
> No need to use *WithTypeCheck() here; you just type-checked it. Use
> 
>   V8{{platform_object.cpp_type}} cppValue =
>
V8{{platform_object.cpp_type}}::toImpl(v8::Handle<v8::Object>::Cast(v8_value));
> 
> instead.

Done.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:56: if (v8Value->IsObject()) {
On 2014/10/28 11:31:38, Jens Widell wrote:
> Spec says "object but not Date or RegExp". If we don't want to follow the spec
> ehre for some reason, at least add a (FIXME) comment about the difference.

Added a FIXME comment.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.cpp:74: if (v8Value->IsNumber()) {
On 2014/10/28 11:31:38, Jens Widell wrote:
> You never convert to number (or boolean). If there's no string type, but there
> is a number (or boolean) type, you should convert to number (or boolean) at
the
> end, rather than throwing a type error.

Good catch! Done.

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

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.h:29: {{member.rvalue_cpp_type}}
getAs{{member.type_name}}();
On 2014/10/28 11:31:38, Jens Widell wrote:
> Might be a good idea to inline these?

Unfortunately, forward declarations are used for interface types (e.g. NodeList)
in this header files so we can't inline these. We can add includes, but I
thought it will slow down build speed.

https://codereview.chromium.org/680193003/diff/1/Source/bindings/templates/un...
Source/bindings/templates/union.h:34: unsigned m_specificTypeFlag;
On 2014/10/28 11:31:38, Jens Widell wrote:
> For bonus points (nice when debugging) you could use a generated (private)
> enumeration type here, I guess.

Done.

[haraken, 2014-10-29 04:59:30]

Mostly looks good.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/generated.gyp (right):

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gyp:153:
'<(SHARED_INTERMEDIATE_DIR)/blink/',

Just help me understand: Why do we need a different directory for --output-dir
and --dictionary-impl-output-dir?

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gyp:157:
'<(bindings_modules_output_dir)/InterfacesInfoModulesIndividual.pickle',

Hmm, "InterfacesInfoModulesIndividual" is a bit confusing. This is not a pickle
that contains information of each individual interface...

It should be something like "OtherInfoForModules", although we want to avoid
"Other". What information will be put into the pickle file other than union
types?

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:1047: if idl_type.is_union_type:

Don't we need to check idl_type.is_dictionary_type ? We can probably use
use_output_parameter_for_result(idl_type).

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:1064: if idl_type.is_union_type:

Ditto.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:313: if idl_type and
(idl_type.is_dictionary or idl_type.is_union_type):

Can we use use_output_parameter_for_result(idl_type) ?

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:749: 'StackAllocatedContainer':
'v8SetReturnValue(info, result)',

StackAllocatedContainer => StackAllocatedParameter ?

We should probably rename 'use_output_parameter_for_result' to
'use_stack_allocated_parameter_for_result' for consistency.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:20: header_forward_decls.clear()

Nit: Do we need these clear()s?

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:60: string_type = context

Add:

  else:
    raise ...

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:88: return
v8::Null(isolate);

Just to confirm: This is v8::Null(isolate), not v8::Undefined(isolate), right?

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:100: return
v8::Null(isolate);

Nit: v8::Null(isolate) => v8::Handle<v8::Value>()

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:277:
RefPtr<TestInterfaceGarbageCollected> cppValue =
V8TestInterfaceGarbageCollected::toImpl(v8::Handle<v8::Object>::Cast(v8Value));

This looks strange and will break compile in oilpan builds.

RefPtr<TestInterfaceGarbageCollected> => TestInterfaceGarbageCollected*

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:409:
RefPtr<TestInterfaceWillBeGarbageCollected> cppValue =
V8TestInterfaceWillBeGarbageCollected::toImpl(v8::Handle<v8::Object>::Cast(v8Value));

RefPtr<TestInterfaceWillBeGarbageCollected> =>
RefPtrWillBeRawPtr<TestInterfaceWillBeGarbageCollected>

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:49: SpecificTypes
m_specificType;

Nit: 'Types m_type' might be enough.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:56: class
V8LongSequenceOrStringArrayOrUnrestrictedDouble {

Add final.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:64: inline void
v8SetReturnValue(const CallbackInfo& callbackInfo,
LongSequenceOrStringArrayOrUnrestrictedDouble& impl)

For now I'm fine with static toImpl(), toV8() and v8SetReturnValue(), but in
long-term we might want to make V8XXXOrYYYOrZZZ inherit from ScriptWrappable and
use virtual toImpl(), toV8() and v8SetReturnValue().

We should probably discuss with shiino-san.

[Jens Widell, 2014-10-29 07:18:51]

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/template...
Source/bindings/templates/union.cpp:39: {# FIXME: We doesn't follow the spec on
handling null and undefined at this
"We don't" :-)

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/template...
Source/bindings/templates/union.cpp:96: {% if container.numeric_type %}
Use "elif" instead here (and for boolean)? We only need the unconditional
fallback code for one of string, number and boolean.

The C++ compiler would of course eliminate the extra (dead) code, but since we
don't need any complicated logic here to only generate one of the fallbacks, we
might as well do it.

(We could also skip the IsBoolean()/IsNumber() checks above when we'd fall back
to that type anyway, but that of course takes a little bit more logic to do.)

[bashi, 2014-10-29 09:54:10]

Patchset #3 (id:60001) has been deleted

[bashi, 2014-10-29 09:57:37]

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/generated.gyp (right):

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gyp:153:
'<(SHARED_INTERMEDIATE_DIR)/blink/',
On 2014/10/29 04:59:29, haraken wrote:
> 
> Just help me understand: Why do we need a different directory for --output-dir
> and --dictionary-impl-output-dir?

Dictionary impls are generated under gen/blink/{core,modules}/foo/bar (e.g.
core/dom/DOMPoint.idl -> gen/blink/core/dom/DOMPoint.{cpp,h}).

Union types are generated under
gen/blink/bindings/{core,modules}/v8/UnionTypes{Core,Modules}.{cpp,h}

We can just pass 'gen/blink' as --output-dir, but this I don't think it is a
good idea because we need to generate something the same as
"bindings_modules_v8_output_dir" in idl_compiler.py. This means we will have two
places to define the directory path.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/generated.gyp:157:
'<(bindings_modules_output_dir)/InterfacesInfoModulesIndividual.pickle',
On 2014/10/29 04:59:29, haraken wrote:
> 
> Hmm, "InterfacesInfoModulesIndividual" is a bit confusing. This is not a
pickle
> that contains information of each individual interface...
> 
> It should be something like "OtherInfoForModules", although we want to avoid
> "Other". What information will be put into the pickle file other than union
> types?

Hmm, in fact, we already put global information into the pickle
file('partial_interface_files'). Anyway, added new pickle files for storing
component-wide information.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:1047: if idl_type.is_union_type:
On 2014/10/29 04:59:30, haraken wrote:
> 
> Don't we need to check idl_type.is_dictionary_type ? We can probably use
> use_output_parameter_for_result(idl_type).

Done. (Though I'm not sure we already support dictionary here; will check later)

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:1064: if idl_type.is_union_type:
On 2014/10/29 04:59:30, haraken wrote:
> 
> Ditto.

Done.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:313: if idl_type and
(idl_type.is_dictionary or idl_type.is_union_type):
On 2014/10/29 04:59:30, haraken wrote:
> 
> Can we use use_output_parameter_for_result(idl_type) ?

Done.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:749: 'StackAllocatedContainer':
'v8SetReturnValue(info, result)',
On 2014/10/29 04:59:30, haraken wrote:
> 
> StackAllocatedContainer => StackAllocatedParameter ?
> 
> We should probably rename 'use_output_parameter_for_result' to
> 'use_stack_allocated_parameter_for_result' for consistency.

Hmm, keys here are 'conversion type' so *Parameter sounds a bit strange. I'd
like to keep it.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:20: header_forward_decls.clear()
On 2014/10/29 04:59:30, haraken wrote:
> 
> Nit: Do we need these clear()s?

Removed.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:60: string_type = context
On 2014/10/29 04:59:30, haraken wrote:
> 
> Add:
> 
>   else:
>     raise ...

We should, but since this CL doesn't support sequence, raise an exception breaks
run-bindings-tests. I'll implement it or update tests in a follow-up CL shortly.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/template...
Source/bindings/templates/union.cpp:39: {# FIXME: We doesn't follow the spec on
handling null and undefined at this
On 2014/10/29 07:18:51, Jens Widell wrote:
> "We don't" :-)

Done. Thank you for always correcting my poor English :)

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/template...
Source/bindings/templates/union.cpp:96: {% if container.numeric_type %}
On 2014/10/29 07:18:51, Jens Widell wrote:
> Use "elif" instead here (and for boolean)? We only need the unconditional
> fallback code for one of string, number and boolean.
> 
> The C++ compiler would of course eliminate the extra (dead) code, but since we
> don't need any complicated logic here to only generate one of the fallbacks,
we
> might as well do it.
> 
> (We could also skip the IsBoolean()/IsNumber() checks above when we'd fall
back
> to that type anyway, but that of course takes a little bit more logic to do.)

Done. I'd like to keep fallback logic at this time for simplicity.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:88: return
v8::Null(isolate);
On 2014/10/29 04:59:30, haraken wrote:
> 
> Just to confirm: This is v8::Null(isolate), not v8::Undefined(isolate), right?

Yes. The first patch set returned undefined, but the spec says it should return
null. Sorry for the confusion.
http://heycam.github.io/webidl/#es-union

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:100: return
v8::Null(isolate);
On 2014/10/29 04:59:30, haraken wrote:
> 
> Nit: v8::Null(isolate) => v8::Handle<v8::Value>()

Done.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:277:
RefPtr<TestInterfaceGarbageCollected> cppValue =
V8TestInterfaceGarbageCollected::toImpl(v8::Handle<v8::Object>::Cast(v8Value));
On 2014/10/29 04:59:30, haraken wrote:
> 
> This looks strange and will break compile in oilpan builds.
> 
> RefPtr<TestInterfaceGarbageCollected> => TestInterfaceGarbageCollected*

Good catch! Done.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:409:
RefPtr<TestInterfaceWillBeGarbageCollected> cppValue =
V8TestInterfaceWillBeGarbageCollected::toImpl(v8::Handle<v8::Object>::Cast(v8Value));
On 2014/10/29 04:59:30, haraken wrote:
> 
> RefPtr<TestInterfaceWillBeGarbageCollected> =>
> RefPtrWillBeRawPtr<TestInterfaceWillBeGarbageCollected>

Done.

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:49: SpecificTypes
m_specificType;
On 2014/10/29 04:59:30, haraken wrote:
> 
> Nit: 'Types m_type' might be enough.

Done.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:56: class
V8LongSequenceOrStringArrayOrUnrestrictedDouble {
On 2014/10/29 04:59:30, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.h:64: inline void
v8SetReturnValue(const CallbackInfo& callbackInfo,
LongSequenceOrStringArrayOrUnrestrictedDouble& impl)
On 2014/10/29 04:59:30, haraken wrote:
> 
> For now I'm fine with static toImpl(), toV8() and v8SetReturnValue(), but in
> long-term we might want to make V8XXXOrYYYOrZZZ inherit from ScriptWrappable
and
> use virtual toImpl(), toV8() and v8SetReturnValue().
> 
> We should probably discuss with shiino-san.

I'm not sure we could have benefits from ScriptWrappable for union types. Their
toImpl()/toV8() won't have similar logic to other union types. Anyway, lets
discuss later with shiino-san.

[Jens Widell, 2014-10-29 11:40:45]

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:749: 'StackAllocatedContainer':
'v8SetReturnValue(info, result)',
On 2014/10/29 09:57:36, bashi1 wrote:
> On 2014/10/29 04:59:30, haraken wrote:
> > 
> > StackAllocatedContainer => StackAllocatedParameter ?
> > 
> > We should probably rename 'use_output_parameter_for_result' to
> > 'use_stack_allocated_parameter_for_result' for consistency.
> 
> Hmm, keys here are 'conversion type' so *Parameter sounds a bit strange. I'd
> like to keep it.

For now, "DictionaryOrUnion" could work. The other conversion types are named
after the type they convert.

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

https://codereview.chromium.org/680193003/diff/80001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:784: IdlUnionType.release = False
This isn't needed; all properties on IdlBaseType sub-classes evaluate to None,
hence false, by default. We already depend on that here for
IdlArrayOrSequenceType.

https://codereview.chromium.org/680193003/diff/80001/Source/bindings/template...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/680193003/diff/80001/Source/bindings/template...
Source/bindings/templates/union.cpp:116: return v8::Null(isolate);
It's possible we should sometimes convert to undefined here. WebIDL has the
(somewhat unclear, to me) text

  "If the value is an object reference to a special object that represents an
ECMAScript undefined value, then it is converted to the ECMAScript undefined
value."

OTOH, if a member type is nullable and this "union value" was the result of
converting an ECMAScript null value, then surely it should convert back to an
ECMAScript null value, so... either might be right or wrong.  :-)

[bashi, 2014-10-30 01:34:37]

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

https://codereview.chromium.org/680193003/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:749: 'StackAllocatedContainer':
'v8SetReturnValue(info, result)',
On 2014/10/29 11:40:45, Jens Widell wrote:
> On 2014/10/29 09:57:36, bashi1 wrote:
> > On 2014/10/29 04:59:30, haraken wrote:
> > > 
> > > StackAllocatedContainer => StackAllocatedParameter ?
> > > 
> > > We should probably rename 'use_output_parameter_for_result' to
> > > 'use_stack_allocated_parameter_for_result' for consistency.
> > 
> > Hmm, keys here are 'conversion type' so *Parameter sounds a bit strange. I'd
> > like to keep it.
> 
> For now, "DictionaryOrUnion" could work. The other conversion types are named
> after the type they convert.

Done.

https://codereview.chromium.org/680193003/diff/80001/Source/bindings/template...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/680193003/diff/80001/Source/bindings/template...
Source/bindings/templates/union.cpp:116: return v8::Null(isolate);
On 2014/10/29 11:40:45, Jens Widell wrote:
> It's possible we should sometimes convert to undefined here. WebIDL has the
> (somewhat unclear, to me) text
> 
>   "If the value is an object reference to a special object that represents an
> ECMAScript undefined value, then it is converted to the ECMAScript undefined
> value."
> 
> OTOH, if a member type is nullable and this "union value" was the result of
> converting an ECMAScript null value, then surely it should convert back to an
> ECMAScript null value, so... either might be right or wrong.  :-)

I missed the statement, thank you for pointing this out.

That said, as we don't distinguish undefined/null in dictionary, I'd like to
leave it as is for now. Added a FIXME comment.

[haraken, 2014-10-30 08:39:27]

I have a question about how toImpl should work :)

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/BU...
File Source/bindings/core/BUILD.gn (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/BU...
Source/bindings/core/BUILD.gn:18:
"$bindings_core_output_dir/ComponentWideInfoCore.pickle"

Nit: ComponentWideInfo => ComponentInfo ?

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:151:
'<(bindings_modules_output_dir)/InterfacesInfoModules.pickle',

Let's rename InterfacesInfoModules.pickle to InterfacesInfoOverall.pickle in a
follow-up CL.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:166: '--dictionary-impl-output-dir',

--dictionary-impl-output-dir => --impl-output-dir ?

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:179: generator =
CodeGeneratorUnionTypeContainers(

CodeGeneratorUnionTypeContainers => CodeGeneratorUnionType ?

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

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:166: return idl_type.name

Can we probably move this down to just below the 'if idl_type.is_dictionary'
branch?

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:84:
TONATIVE_VOID_EXCEPTIONSTATE(double, cppValue, toDouble(v8Value,
exceptionState), exceptionState);

This conversion looks wrong. "LongSequence" and "StringArray" is going to be
converted to double.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:211: if
(v8Value->IsNumber()) {

I'm getting a bit confused how toImpl works.

When a "String or double" value is given, for what types should we try to
convert the value into String, and for what types should we try to convert the
value into double?

Currently you're trying to convert only IsNumber values into double, but is it a
speced behavior?

In general, my question is, when an "X or Y" value is given, when should we
convert the value into X and when should we convert the value to Y?

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:218:
TOSTRING_VOID(V8StringResource<>, cppValue, v8Value);

Just to confirm: It is OK to not pass exceptionState to TOSTRING_VOID, right?

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/UnionTypesCore.h (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.h:49: SpecificTypes m_type;

Nit: SpecificTypes => Types ?

[Jens Widell, 2014-10-30 08:49:14]

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:211: if
(v8Value->IsNumber()) {
On 2014/10/30 08:39:26, haraken wrote:
> 
> I'm getting a bit confused how toImpl works.
> 
> When a "String or double" value is given, for what types should we try to
> convert the value into String, and for what types should we try to convert the
> value into double?
> 
> Currently you're trying to convert only IsNumber values into double, but is it
a
> speced behavior?
> 
> In general, my question is, when an "X or Y" value is given, when should we
> convert the value into X and when should we convert the value to Y?

See: http://heycam.github.io/webidl/#es-union (primitives are handled at the
end)

The algorithm is essentially the same as for overload resolution.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:218:
TOSTRING_VOID(V8StringResource<>, cppValue, v8Value);
On 2014/10/30 08:39:26, haraken wrote:
> 
> Just to confirm: It is OK to not pass exceptionState to TOSTRING_VOID, right?

Ah, good catch. No, I'd say it's not OK here, since this method takes an
exceptionState parameter. Since it does, it's supposed to throw everything via
that parameter.

So we should use the V8StringResource::prepare(ExceptionState&) here. There's no
macro for that in V8BindingMacros.h (except one for the Promise case) but we
could of course add that. We'd also need to extend the code in v8_types.py to
support enforcing use of ExceptionState even for strings, which it normally
doesn't.

[bashi, 2014-10-30 11:09:39]

Patchset #5 (id:120001) has been deleted

[bashi, 2014-10-30 12:20:48]

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/BU...
File Source/bindings/core/BUILD.gn (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/BU...
Source/bindings/core/BUILD.gn:18:
"$bindings_core_output_dir/ComponentWideInfoCore.pickle"
On 2014/10/30 08:39:26, haraken wrote:
> 
> Nit: ComponentWideInfo => ComponentInfo ?

Done.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/generated.gyp (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:151:
'<(bindings_modules_output_dir)/InterfacesInfoModules.pickle',
On 2014/10/30 08:39:26, haraken wrote:
> 
> Let's rename InterfacesInfoModules.pickle to InterfacesInfoOverall.pickle in a
> follow-up CL.

Sure, but I'd like to ask tasak@ for his opinion about the rename.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/generated.gyp:166: '--dictionary-impl-output-dir',
On 2014/10/30 08:39:26, haraken wrote:
> 
> --dictionary-impl-output-dir => --impl-output-dir ?

Done.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
File Source/bindings/scripts/idl_compiler.py (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/idl_compiler.py:179: generator =
CodeGeneratorUnionTypeContainers(
On 2014/10/30 08:39:26, haraken wrote:
> 
> CodeGeneratorUnionTypeContainers => CodeGeneratorUnionType ?

Done.

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

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:166: return idl_type.name
On 2014/10/30 08:39:26, haraken wrote:
> 
> Can we probably move this down to just below the 'if idl_type.is_dictionary'
> branch?

IdlUnionType doesn't have base_type, so we need to put this check here.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:84:
TONATIVE_VOID_EXCEPTIONSTATE(double, cppValue, toDouble(v8Value,
exceptionState), exceptionState);
On 2014/10/30 08:39:26, haraken wrote:
> 
> This conversion looks wrong. "LongSequence" and "StringArray" is going to be
> converted to double.

Yes, this is wrong as I didn't implement sequences and arrays. I removed union
types which have arrays and sequences from testing IDL files since we don't
support them yet.

As for this fallback number conversion, this is a spec-ed conversion (step 17)
as Jens described.

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:211: if
(v8Value->IsNumber()) {
On 2014/10/30 08:49:14, Jens Widell wrote:
> On 2014/10/30 08:39:26, haraken wrote:
> > 
> > I'm getting a bit confused how toImpl works.
> > 
> > When a "String or double" value is given, for what types should we try to
> > convert the value into String, and for what types should we try to convert
the
> > value into double?
> > 
> > Currently you're trying to convert only IsNumber values into double, but is
it
> a
> > speced behavior?
> > 
> > In general, my question is, when an "X or Y" value is given, when should we
> > convert the value into X and when should we convert the value to Y?
> 
> See: http://heycam.github.io/webidl/#es-union (primitives are handled at the
> end)
> 
> The algorithm is essentially the same as for overload resolution.

Thank you Jens for the explanation!

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:218:
TOSTRING_VOID(V8StringResource<>, cppValue, v8Value);
On 2014/10/30 08:49:14, Jens Widell wrote:
> On 2014/10/30 08:39:26, haraken wrote:
> > 
> > Just to confirm: It is OK to not pass exceptionState to TOSTRING_VOID,
right?
> 
> Ah, good catch. No, I'd say it's not OK here, since this method takes an
> exceptionState parameter. Since it does, it's supposed to throw everything via
> that parameter.
> 
> So we should use the V8StringResource::prepare(ExceptionState&) here. There's
no
> macro for that in V8BindingMacros.h (except one for the Promise case) but we
> could of course add that. We'd also need to extend the code in v8_types.py to
> support enforcing use of ExceptionState even for strings, which it normally
> doesn't.

Yeah, good catch. Done. (though v8_value_to_local_cpp_value getting worse.. I'll
refactor it in a follow-up CL).

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/UnionTypesCore.h (right):

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.h:49: SpecificTypes m_type;
On 2014/10/30 08:39:26, haraken wrote:
> 
> Nit: SpecificTypes => Types ?

'specific type' is a spec-defined term and it would be helpful so I'd like to
call it SpecificTypes.

[Jens Widell, 2014-10-30 12:31:50]

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

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:166: return idl_type.name
On 2014/10/30 12:20:48, bashi1 wrote:
> On 2014/10/30 08:39:26, haraken wrote:
> > 
> > Can we probably move this down to just below the 'if idl_type.is_dictionary'
> > branch?
> 
> IdlUnionType doesn't have base_type, so we need to put this check here.

Isn't idl_type.base_type simply None for union types due to
IdlTypeBase.__getattr__()? That would make it fall through all the specific
checks.

[haraken, 2014-10-30 13:01:30]

LGTM. Let's wait for Jens' approval.

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

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/scripts...
Source/bindings/scripts/v8_union.py:31: # These variables refer member contexts
if the given union type has

refer => refer to

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
Source/bindings/templates/union.cpp:45: {# The numbers in the following comments
refer the steps described in

refer => refer to

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
Source/bindings/templates/union.cpp:59: {# FIXME: This should be "object but not
Date or RegExp". Adds checks when

Adds => Add

[Jens Widell, 2014-10-30 14:04:07]

LGTM too. Great work!

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
Source/bindings/templates/union.cpp:98: return;
Aren't we missing an

  impl.set{{container.numeric_type.type_name}}(cppValue);

here?

[bashi, 2014-10-30 23:19:19]

Thank you for reviewing this large CL! I'm going to land this CL.

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

https://codereview.chromium.org/680193003/diff/100001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:166: return idl_type.name
On 2014/10/30 12:31:50, Jens Widell wrote:
> On 2014/10/30 12:20:48, bashi1 wrote:
> > On 2014/10/30 08:39:26, haraken wrote:
> > > 
> > > Can we probably move this down to just below the 'if
idl_type.is_dictionary'
> > > branch?
> > 
> > IdlUnionType doesn't have base_type, so we need to put this check here.
> 
> Isn't idl_type.base_type simply None for union types due to
> IdlTypeBase.__getattr__()? That would make it fall through all the specific
> checks.

You are right. Done.

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

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/scripts...
Source/bindings/scripts/v8_union.py:31: # These variables refer member contexts
if the given union type has
On 2014/10/30 13:01:30, haraken wrote:
> 
> refer => refer to

Done.

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
File Source/bindings/templates/union.cpp (right):

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
Source/bindings/templates/union.cpp:45: {# The numbers in the following comments
refer the steps described in
On 2014/10/30 13:01:30, haraken wrote:
> 
> refer => refer to

Done.

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
Source/bindings/templates/union.cpp:59: {# FIXME: This should be "object but not
Date or RegExp". Adds checks when
On 2014/10/30 13:01:30, haraken wrote:
> 
> Adds => Add

Done.

https://codereview.chromium.org/680193003/diff/140001/Source/bindings/templat...
Source/bindings/templates/union.cpp:98: return;
On 2014/10/30 14:04:07, Jens Widell wrote:
> Aren't we missing an
> 
>   impl.set{{container.numeric_type.type_name}}(cppValue);
> 
> here?

Yes. Thank you for taking a closer look as usual!

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/tests/r...
Source/bindings/tests/results/core/UnionTypesCore.cpp:355: if
(v8Value->IsNumber()) {
As Jens suggested, we should be able to remove this. I may try it when I
implement all conversions (arrays, etc).

[bashi, 2014-10-30 23:29:34]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-10-30 23:30:07]

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

[commit-bot: I haz the power, 2014-10-31 01:50:08]

Committed patchset #6 (id:160001) as 184684

[sof, 2014-10-31 06:48:19]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-10-31 06:48:21]

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/templat...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/templat...
Source/bindings/templates/union.h:22: ALLOW_ONLY_INLINE_ALLOCATION();
Looks like this doesn't generate a trace() method, which is required.


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

[sof, 2014-10-31 06:52:23]

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/templat...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/templat...
Source/bindings/templates/union.h:22: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/10/31 06:48:20, sof wrote:
> Looks like this doesn't generate a trace() method, which is required.
> 
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

Just to be clear(er), 

  s/required/required if the generated type has one or more Member<>s/

[Jens Widell, 2014-10-31 07:24:02]

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/templat...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/680193003/diff/160001/Source/bindings/templat...
Source/bindings/templates/union.h:22: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/10/31 06:52:23, sof wrote:
> On 2014/10/31 06:48:20, sof wrote:
> > Looks like this doesn't generate a trace() method, which is required.
> > 
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
> 
> Just to be clear(er), 
> 
>   s/required/required if the generated type has one or more Member<>s/

https://codereview.chromium.org/689013002/