Bug 111699 – [V8] generate indexedPropertyGetter instead of using collectionStringIndexedPropertyGetter()

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[foo]

[V8] generate indexedPropertyGetter instead of using collectionStringIndexedPropertyGetter()

[foo]

Created attachment 192126 [details]
Patch

[Kentaro Hara]

Comment on attachment 192126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192126&action=review

> Source/WebCore/ChangeLog:14
> +        No new tests: code refactoring.

You need to update run-bindings-tests. See https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2500
> +    # FIXME: Clarify condition when enumerator should be enabled.
> +    # Basically an interface that has indexedPropertyGetter must have indexedPropertyEnumerator but currently there are some special cases.

A bit better:

  FIXME: Remove the special cases. Interfaces that have indexedPropertyGetter should have indexedPropertyEnumerator.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2526
> +    # FIXME: Remove this block to use generated indexed getter instead of halfway ones such as V8Collection::collectionXXXIndexedPropertyGetter().

FIXME: This block is full of hacks to incrementally replace custom indexed getters with auto-generated ones. Remove this block and V8Collection.h.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2566
> +    # FIXME: Clarify condition for indexedPropertyEnumerator to be available then implement it.

I think you can remove the FIXME. You already wrote the same thing above.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3291
> +        if (!$indexer) {
> +            $indexer = $codeGenerator->FindSuperMethod($interface, "item");
> +        }

Remove this. This is already done above.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3294
> +        # FIXME: Figure out what NumericIndexedGetter is really supposed to do. Right now, it's only set on WebGL-related files.

Remove the FIXME. This is already written above. (Let's kill NumericIndexedGetter in the near future.)

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

I think you can remove the branch by using NativeToJSValue().

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3300
> +                my $conversion = $indexer->extendedAttributes->{"TreatReturnedNullStringAs"};
> +                if ($conversion && $conversion eq "Null") {
> +                    # FIXME: generate indexedPropertyGetter instead of generating setCollectionStringOrUndefinedIndexedGetter() call in GenerateImplementationIndexer.

NativeToJSValue() will care about this.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3309
> +    return v8String(result, info.GetIsolate());

This part should be generated by NativeToJSValue().

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3315
> +            } else {
> +                # FIXME: generate indexedPropertyGetter instead of generating setCollectionIndexedGetter() call in GenerateImplementationIndexer.
> +            }

NativeToJSValue() will care about this.

[Kentaro Hara]

Please block bug 111810 when you work on indexed properties and named properties so that people can track your work easily.

[foo]

Created attachment 196280 [details]
Patch

[Kentaro Hara]

Comment on attachment 196280 [details]
Patch

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.)

[foo]

Created attachment 196289 [details]
Patch

[foo]

Added index bounds check instead of null check.
Checking how many tests will fail...

[foo]

Extra explanation:
Current code is checking index out of range by collection->item(index) is NULL or not.
This means if collection->item(index) returns NULL for valid index, index getter returns undefined.
So current code is not good because index getter should return same value as item() method for valid index.

After I added index bound check, 3 tests newly failed for run_webkit_tests.sh.

[WebKit Review Bot]

Comment on attachment 196289 [details]
Patch

Attachment 196289 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17224720

New failing tests:
fast/dom/CSSStyleDeclaration/css-computed-style-item.html
fast/dom/CSSStyleDeclaration/css-style-item.html
gamepad/gamepad-polling-access.html

[WebKit Review Bot]

Created attachment 196298 [details]
Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Anders Carlsson]

V8 is gone.