Support union return type for anonymous named/indexed getter

Support union return type for anonymous named/indexed getter

Currently union return type is treated in custom binding.
To auto-generate that,
- create UnionType
- create function which returns UnionType in webcore
- call that function from namedPropertyGetter in binding code 

Next step:
- Support union return type for general method

This patch also replace custom named getter
of V8HTMLFormControlsCollection with auto generated one.


R=haraken@chromium.org
BUG=240176
TEST=TestEventTarget.idl, TestObject.idl
NOTRY=true

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

[kojih, 2013-05-15 09:28:26]

r?

https://codereview.chromium.org/15076011/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15076011/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:3321: return jsValue;
I had to create "create and set JSValue" statement
because UnionType can not be obtained as expression.

(that is because branch is needed to test which union member is available)

https://codereview.chromium.org/15076011/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:5027: my $extendedAttributes = shift;
Split signature into type and extendedAttributes
because it have to call each union type with common extendedAttributes, to treat
union type.

https://codereview.chromium.org/15076011/diff/1/Source/wtf/UnionType.h
File Source/wtf/UnionType.h (right):

https://codereview.chromium.org/15076011/diff/1/Source/wtf/UnionType.h#newcode1
Source/wtf/UnionType.h:1: #ifndef UnionType_h
I don't confident where to place this file.
Do you have any idea?

[haraken, 2013-05-15 10:56:25]

The approach looks good. The union type will enable us to remove more custom
bindings!

Below is the first round of comments.

https://codereview.chromium.org/15076011/diff/5001/Source/bindings/tests/resu...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/15076011/diff/5001/Source/bindings/tests/resu...
Source/bindings/tests/results/V8TestObject.cpp:4315: return jsValue;

Can we write this like:

  return toV8Fast(element.enabledValue(), info, collection);

?

https://codereview.chromium.org/15076011/diff/5001/Source/bindings/tests/resu...
Source/bindings/tests/results/V8TestObject.cpp:4334: return jsValue;

Nit: We might not want to introduce the jsValue indirection (just for
readability).

https://codereview.chromium.org/15076011/diff/5001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8HTMLFormControlsCollectionCustom.cpp (left):

https://codereview.chromium.org/15076011/diff/5001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8HTMLFormControlsCollectionCustom.cpp:68:
v8::Handle<v8::Value> V8HTMLFormControlsCollection::namedItemMethodCustom(const
v8::Arguments& args)

You want to remove this method in a follow-up patch.

https://codereview.chromium.org/15076011/diff/5001/Source/core/html/HTMLFormC...
File Source/core/html/HTMLFormControlsCollection.idl (right):

https://codereview.chromium.org/15076011/diff/5001/Source/core/html/HTMLFormC...
Source/core/html/HTMLFormControlsCollection.idl:26: [ImplementedAs=namedGetter]
getter (RadioNodeList or Node)(DOMString name);

Looks great!

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h
File Source/wtf/UnionType.h (right):

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h#new...
Source/wtf/UnionType.h:1: #ifndef UnionType_h

You need to add a copyright.

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h#new...
Source/wtf/UnionType.h:7: class UnionType2 {

Let's name it UnionMemberType, per the Web IDL spec:
http://www.w3.org/TR/WebIDL/#idl-grammar

Also please add a comment about what UnionMemberType is for.

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h#new...
Source/wtf/UnionType.h:12: UnionType2(PassRefPtr<Type0> value)

Please add 'explicit'.

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h#new...
Source/wtf/UnionType.h:16: UnionType2(PassRefPtr<Type1> value)
Ditto.

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h#new...
Source/wtf/UnionType.h:24: bool isValue0Enabled() const
How about just adding enabledValue()? Then neither isValue0Enabled() nor
getValue0() will be needed.

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h#new...
Source/wtf/UnionType.h:41: RefPtr<Type0> value0;

m_value0

https://codereview.chromium.org/15076011/diff/5001/Source/wtf/UnionType.h#new...
Source/wtf/UnionType.h:42: RefPtr<Type1> value1;
 m_value1

[kojih, 2013-05-15 11:30:19]

diff of generated code is:

+v8::Handle<v8::Value>
V8HTMLFormControlsCollection::namedPropertyGetter(v8::Local<v8::String> name,
const v8::AccessorInfo& info)
+{
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
+        return v8Undefined();
+    if (info.Holder()->HasRealNamedCallbackProperty(name))
+        return v8Undefined();
+
+    ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
+    HTMLFormControlsCollection* collection = toNative(info.Holder());
+    AtomicString propertyName = toWebCoreAtomicString(name);
+    WTF::UnionType2<RadioNodeList, Node> element =
collection->namedGetter(propertyName);
+    if (element.isNull())
+        return v8Undefined();
+    if (element.enabledValue0())
+        return toV8Fast(element.enabledValue0(), info, collection);
+    if (element.enabledValue1())
+        return toV8Fast(element.enabledValue1(), info, collection);
+    return v8Undefined();
+
+}
+

[haraken, 2013-05-15 12:09:43]

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:1645: $code .= "    return " .
NativeToJSValue($attribute->signature->type,
$attribute->signature->extendedAttributes, $expression, "info.Holder()",
"info.GetIsolate()", "info", "imp", "ReturnUnsafeHandle",
$forMainWorldSuffix).";\n";

You can pass $attribute->signature instead of passing
$attribute->signature->type and $attribute->signature->extendedAttributes.

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3167: sub GetReturnJSValueStatements

This looks like a duplication of NativeToJSValue(). I think you anyway need to
merge GetReturnJSValueStatements() with NativeToJSValue() at some point where
you want to kill more custom bindings by introducing UnionTypes. So, can we
avoid the duplication now?

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3183: if (ref $type) {

'ref $type' is hard to understand. Let's implement a helper method
IsUnionType().

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:4690: die "Currently only 2 values of
non-union type is supported as union type.\n" unless scalar(@$types)==2 &&
scalar(grep {ref $_} @$types)==0;

Nit: 'scalar(@$types) == 2' would be enough in practice. (For correctness, you
need to check a lot of more things, such as whether a given type is a RefPtr)

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:5024: my $value = shift; # expression
of native value

$value => $nativeValue

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1828: return $type;

Is it OK to return a string? Shouldn't parseUnionMemberType return a
UnionMemberType?

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1831: return $self->parseNonAnyType();

Ditto. parseNonAnyType() returns nothing.

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1847: return ();

Nit: You can write:

  my @types = ();
  if (...) {
    ...;
  }
  return @types;

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestObject.cpp:4310: if
(element.enabledValue1())

Nit: if => else if (or else)

https://codereview.chromium.org/15076011/diff/12001/Source/wtf/UnionType.h
File Source/wtf/UnionType.h (right):

https://codereview.chromium.org/15076011/diff/12001/Source/wtf/UnionType.h#ne...
Source/wtf/UnionType.h:2: * Copyright (C) 2009, 2011 Apple Inc. All rights
reserved.

Please use this:

/*
 * Copyright (C) 2013 Google Inc. All rights reserved.
 * 
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions are
 * met:
 * 
 *     * Redistributions of source code must retain the above copyright
 * notice, this list of conditions and the following disclaimer.
 *     * Redistributions in binary form must reproduce the above
 * copyright notice, this list of conditions and the following disclaimer
 * in the documentation and/or other materials provided with the
 * distribution.
 *     * Neither the name of Google Inc. nor the names of its
 * contributors may be used to endorse or promote products derived from
 * this software without specific prior written permission.
 * 
 * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
 * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
 * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
 * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
 * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
 * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
 * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
 * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */

https://codereview.chromium.org/15076011/diff/12001/Source/wtf/UnionType.h#ne...
Source/wtf/UnionType.h:32: * UnionTypeN holds multiple types of objects.

Nit: Blink uses // for multiline comments.

https://codereview.chromium.org/15076011/diff/12001/Source/wtf/UnionType.h#ne...
Source/wtf/UnionType.h:43: explicit UnionType2()

'explicit' is needed only for one-argument constructor.

[jsbell, 2013-05-15 21:37:52]

This is awesome! Thanks for taking this on.

Just a couple of drive-by comments.

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestObject.cpp:4310: if
(element.enabledValue1())
On 2013/05/15 12:09:43, haraken wrote:
> 
> Nit: if => else if (or else)

This is generated in a loop... IMHO the if/return style is more readable here
and for the generator.

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestObject.cpp:4312: return v8Undefined();
But maybe generate a ASSERT_NOT_REACHED() before the return?

https://codereview.chromium.org/15076011/diff/12001/Source/wtf/UnionType.h
File Source/wtf/UnionType.h (right):

https://codereview.chromium.org/15076011/diff/12001/Source/wtf/UnionType.h#ne...
Source/wtf/UnionType.h:58: PassRefPtr<Type0> enabledValue0() const
Naming suggestion: in Web IDL, a UnionType is composed of UnionMemberTypes. And
humans start counting at 1. So perhaps member1(), member2() etc?

[kojih, 2013-05-16 05:22:57]

haraken:
r?

jsbell:
Reflected your comments.
Thank you!

Diff of generated code is:

--- /mnt/hdd/diff/gen_stmt/V8HTMLFormControlsCollection.cpp     2013-05-16
12:13:53.554657064 +0900
+++ /mnt/hdd/diff/gen_poly/V8HTMLFormControlsCollection.cpp     2013-05-16
14:06:56.286884802 +0900
@@ -24,6 +24,7 @@
 #include "RuntimeEnabledFeatures.h"
 #include "V8HTMLCollection.h"
 #include "V8Node.h"
+#include "V8RadioNodeList.h"
 #include "bindings/v8/ScriptController.h"
 #include "bindings/v8/V8Binding.h"
 #include "bindings/v8/V8Collection.h"
@@ -33,6 +34,9 @@
 #include "core/dom/ContextFeatures.h"
 #include "core/dom/Document.h"
 #include "core/page/Frame.h"
+#include "wtf/GetPtr.h"
+#include "wtf/RefCounted.h"
+#include "wtf/RefPtr.h"
 #include "wtf/UnusedParam.h"
 
 namespace WebCore {
@@ -89,6 +93,27 @@
     return toV8Fast(element.release(), info, collection);
 }
 
+v8::Handle<v8::Value>
V8HTMLFormControlsCollection::namedPropertyGetter(v8::Local<v8::String> name,
const v8::AccessorInfo& info)
+{
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
+        return v8Undefined();
+    if (info.Holder()->HasRealNamedCallbackProperty(name))
+        return v8Undefined();
+
+    ASSERT(V8DOMWrapper::maybeDOMWrapper(info.Holder()));
+    HTMLFormControlsCollection* collection = toNative(info.Holder());
+    AtomicString propertyName = toWebCoreAtomicString(name);
+    WTF::UnionType2<RadioNodeList, Node> element =
collection->namedGetter(propertyName);
+    if (element.isNull())
+        return v8Undefined();
+    if (element.member1())
+        return toV8Fast(element.member1(), info, collection);
+    if (element.member2())
+        return toV8Fast(element.member2(), info, collection);
+    ASSERT_NOT_REACHED();
+    return v8Undefined();
+}
+

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1828: return $type;
On 2013/05/15 12:09:43, haraken wrote:
> 
> Is it OK to return a string? Shouldn't parseUnionMemberType return a
> UnionMemberType?

now parseUnionMemberType returns UnionType or string.

https://codereview.chromium.org/15076011/diff/12001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1831: return $self->parseNonAnyType();
On 2013/05/15 12:09:43, haraken wrote:
> 
> Ditto. parseNonAnyType() returns nothing.

parseNonAnyType returns string.
last line of parseNonAnyType is assertion.
$self->assertUnexpectedToken($next->value(), __LINE__);

https://codereview.chromium.org/15076011/diff/22001/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15076011/diff/22001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:1606: $code .=
NativeToJSValue($attribute->signature->type,
$attribute->signature->extendedAttributes, $expression, "    ", "return",
"info.Holder()", "info.GetIsolate()", "info", "imp", "ReturnUnsafeHandle",
$forMainWorldSuffix) . "\n";
I have to split signature into signature->type and signature->extendedAttributes
because NativeToJSValue have to call NativeToJSValue($unionMemberType,
$extendedAttributes) in case of UnionType.

[kojih, 2013-05-16 05:24:28]

please see patch set 9.

[haraken, 2013-05-16 05:38:05]

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3178: my $nativeExpression =
"element";

Nit: $nativeExpression => $nativeValue

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3181: my $returnJSValueCode =
NativeToJSValue($indexedGetterfunction->signature->type,
$indexedGetterfunction->signature->extendedAttributes, $nativeExpression, "   
", "return", "info.Holder()", "info.GetIsolate()", "info", "collection", "",
"");

Nit: You can omit the last two ""s.

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3220: my $nativeExpression =
"element";

Nit: $nativeExpression => $nativeValue

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3223: my $returnJSValueCode =
NativeToJSValue($namedGetterFunction->signature->type,
$namedGetterFunction->signature->extendedAttributes, $nativeExpression, "    ",
"return", "info.Holder()", "info.GetIsolate()", "info", "collection", "", "");

Nit: You can omit the ""s.

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:4456: die "Currently only 2 values of
non-union type is supported as union type.\n" unless scalar(@$types)==2;

Nit: Spaces are needed around ==.

You can add this check in IsUnionType().

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:4792: die "Currently only 2 values of
non-union type is supported as union type.\n" unless scalar(@$types)==2;

You can move the check to IsUnionType().

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1793: push @{$unionType->unionMemberTypes},
$self->parseUnionMemberTypes();

Is this correct for three or more UnionTypes? Remember that
$self->parseUnionMemberTypes() returns an array. I guess we will end up [type,
type, [type, type, [type, type, [...]]]].

https://codereview.chromium.org/15076011/diff/19003/Source/wtf/UnionType.h
File Source/wtf/UnionType.h (right):

https://codereview.chromium.org/15076011/diff/19003/Source/wtf/UnionType.h#ne...
Source/wtf/UnionType.h:71: RefPtr<Type2> m_value2;

Nit: m_value1 => m_member1

[kojih, 2013-05-16 05:51:20]

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/15076011/diff/19003/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1793: push @{$unionType->unionMemberTypes},
$self->parseUnionMemberTypes();

I confirmed following.

interface TestInterface {
    [ImplementedAs=getItem] getter (Node or NodeList or Element)(DOMString
name);
};

become

$VAR1 = bless( {
                 'UnionType::unionMemberTypes' => [
                                                    'Node',
                                                    'NodeList',
                                                    'Element'
                                                  ]
               }, 'UnionType' );

#List in Perl is strange :)

[haraken, 2013-05-16 05:52:45]

OK, LGTM with nits mentioned above!

[commit-bot: I haz the power, 2013-05-16 10:20:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/15076011/35001

[kojih, 2013-05-16 10:26:21]

eseidel:
Hi, I added Source/wtf/UnionType.h
Is it OK?

[commit-bot: I haz the power, 2013-05-16 10:34:00]

Retried try job too often on blink_bare_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_bare...

[kojih, 2013-05-17 01:39:09]

On 2013/05/16 10:26:21, kojih wrote:
> eseidel:
> Hi, I added Source/wtf/UnionType.h
> Is it OK?

How about you, morita-san?
I need LGTM from one of wtf owners.
Or please let me know other appreciate place to put it.

[kojih, 2013-05-17 01:43:03]

appropriate place

[kojih, 2013-05-17 04:38:41]

as discussed in offline with morita-san, I decided to use output arguments
instead of UnionType2.
So now UnionType.h is not necessary.

[kojih, 2013-05-17 05:13:18]

haraken:
Changed to use output arguments instead of UnionType2.
r?

[haraken, 2013-05-17 05:44:48]

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3063: sub GetUnionMemberVariableName

This method can simply return '"member" . $index'. Then you can write it inline
in the caller sites.

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3071: sub GetIsNullExpression

Nit: GetIsNullExpression => GenerateIsNullExpression

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3093: sub GetIsNotNullExpression

I don't think we need this. We can simply use:

  "!" . GenerateIsNullExpression()

though it sometimes generates something like !!element0. Compilers will remove
the redundancy. Then you don't need GetIsNutNullExpression().

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3174: }

You can remove this. (See my comment in V8TestInterface.cpp.)

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3243: sub GetCallMethodCode

Nit: GetCallMethodCode => GenerateMethodCall

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3288: }

You can remove this. (See my comment in V8TestInterface.cpp.)

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3308: }

You can remove this. (See my comment in V8TestInterface.cpp.)

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:4505: return "";

Is it correct to return ""? Or should this simply be 'die "UnionType is not
supported" if IsUnionType($type)' ?

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:4734: die "Currently only 2 values of
non-union type is supported as union type.\n" unless
scalar(@{$type->unionMemberTypes}) == 2;

Nit: 'scalar' is not needed.

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:4845: push @codes, $code;

You can put 'return v8Undefined()' at the last of the generated code. (See my
comment in V8TestInterface.cpp.)

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/tests/res...
File Source/bindings/tests/results/V8TestInterface.cpp (right):

https://codereview.chromium.org/15076011/diff/44002/Source/bindings/tests/res...
Source/bindings/tests/results/V8TestInterface.cpp:543: ASSERT_NOT_REACHED();

Can you write this like:

  if (elementMember1)
    return ...;
  if (elementMember2)
    return ...;
  return v8Undefined();

?

https://codereview.chromium.org/15076011/diff/44002/Source/core/html/HTMLForm...
File Source/core/html/HTMLFormControlsCollection.h (right):

https://codereview.chromium.org/15076011/diff/44002/Source/core/html/HTMLForm...
Source/core/html/HTMLFormControlsCollection.h:46: void namedGetter(const
AtomicString& name, RefPtr<RadioNodeList>& returnValue1, RefPtr<Node>&
returnValue2);

Nit: You can omit variable names.

[kojih, 2013-05-17 06:15:14]

fixed.

[haraken, 2013-05-17 07:10:36]

LGTM with nits.

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3079: my $unionMemberVariable =
$variableName . GetUnionMemberVariableName($i);

Nit: You can write this like:

  my $unionMemberVariable = $variableName . $i;

Then you can remove GetUnionMemberVariableName().

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3146: my $callMethodCode =
GenerateMethodCall($returnType, "element", "collection->${methodName}",
"index");

Nit: $callMethodCode => $methodCallCode

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3158: END

Nit: You don't need to use here document just for two lines of code.

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3164: END
Ditto.

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3169: END
Ditto.

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3246: my $unionMemberVariable =
$returnName . GetUnionMemberVariableName($i);

Nit: my $unionMemberVariable = $returnName . $i;

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3273: my $callMethodCode =
GenerateMethodCall($returnType, "element", "collection->${methodName}",
"propertyName");

Nit: $methodCallCode

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3292: END

Nit: Here document might not be needed.

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3298: END
Ditto.

https://codereview.chromium.org/15076011/diff/51001/Source/bindings/scripts/C...
Source/bindings/scripts/CodeGeneratorV8.pm:3303: END
Ditto.

[commit-bot: I haz the power, 2013-05-17 07:38:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/15076011/53002

[commit-bot: I haz the power, 2013-05-17 08:34:51]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-05-17 09:29:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/15076011/53002

[kojih, 2013-05-17 09:29:28]

NOTRY=TRUE

[kojih, 2013-05-17 09:32:19]

NO TRY=TRUE

[commit-bot: I haz the power, 2013-05-17 09:48:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kojih@chromium.org/15076011/53002

[commit-bot: I haz the power, 2013-05-17 09:49:07]

Change committed as 150564