[webkit-reviews] review denied: [Bug 111699] [V8] generate indexedPropertyGetter instead of using collectionStringIndexedPropertyGetter() : [Attachment 196280] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Koji Hara <kojih at chromium.org>'s
request for review:
Bug 111699: [V8] generate indexedPropertyGetter instead of using
collectionStringIndexedPropertyGetter()
https://bugs.webkit.org/show_bug.cgi?id=111699

Attachment 196280: Patch
https://bugs.webkit.org/attachment.cgi?id=196280&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=196280&action=review


Basically the approach looks good. Thanks for the patch.

What real IDL files will be affected by this change?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3423
> +	   my $indexerType = $indexer ? $indexer->type : 0;
> +

Remove this. You already did it above.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3435
> +	       my $nullCheck;

You can remove $nullCheck. See the comment below.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3437
> +	       if ($indexerType eq "DOMString") {

You don't need to treat DOMString specially. You can get $elemType by using
GetNativeTypeForCallbacks().

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3445
> +		   if ($interfaceName eq "WebKitCSSKeyframesRule") {
> +		       $jsValue = "toV8(element.release(), info.Holder(),
info.GetIsolate())";

Let's remove this hack in a follow-up patch. It's OK for now.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3454
> +    // FIXME: assert that object must be a collection type

Why do you think we need the check here? In other DOM getters/setters/methods,
we retrieve C++ pointers with toNative(info.Holder()) without checking its
type. This is OK because the type check is already done when V8 looks up a
prototype chain.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3462
> +    // According to spec, JS must return undefined in case of index out of
range.
> +    // see http://www.w3.org/TR/WebIDL/#idl-indexed-properties
> +    if ($nullCheck)
> +	   return v8Undefined();

You should check whether a given index is out of range or not by comparing the
index with collection->length(). You shouldn't rely on whether a returned value
is null or not. (The return value can be null for a valid index.)