Bug 110852 – [V8] Added V8CustomIndexedGetter to IDLs that correspond to existing custom indexedPropertyGetters.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[foo]

[WIP] [V8] Generate IndexGetter bindings

[foo]

Created attachment 190218 [details]
Patch

[Kentaro Hara]

Comment on attachment 190218 [details]
Patch

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

Thanks for the first patch!

> Source/WebCore/ChangeLog:7
> +

Please describe what this patch is doing.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3259
> +        my @customClassess = (
> +            "V8DOMWindow",
> +            "V8HTMLAppletElement",
> +            "V8HTMLEmbedElement",
> +            "V8HTMLObjectElement",
> +            "V8Storage",
> +            "V8HTMLFormElement",   # Irregular: RefPtr<Node> formElement = form->elements()->item(index);
> +        );

We don't want to hard code class names in code generators. Alternately, you can replace [IndexedGetter] of these IDL files with [V8CustomIndexedGetter] to indicate that they are written in custom bindings. (Then our goal is to replace the [V8CustomIndexedGetter] with [IndexedGetter] as much as possible.)

(There are a lot of hard-coded class names in code generators. We've been trying hard to remove them.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4432
> +    return 1 if $type eq "CSSStyleDeclaration" or $type eq "MediaList" or $type eq "DOMStringList" or $type eq "DOMTokenList" or $type eq "DOMSettableTokenList";

Ditto. We should control whether the binding code is generated by IDL attributes, not by hard-coding something in code generators.

[foo]

Created attachment 190462 [details]
Patch

[Kentaro Hara]

Comment on attachment 190462 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Generate IndexedGetter from IDL files, instead of using custom implementation.

You can write the description between the 'Reviewed by' line and the 'Test:' line.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3252
> +    if($interface->extendedAttributes->{"IndexedGetter"}
> +       && $itemReturnType ne ""
> +       && $itemReturnType ne "DOMString") {

This change isn't quite right. e.g. what happens if there is an interface that has [IndexedGetter] and returns a DOMString but needs a custom binding?

I think the right way to fix the issue would be:

(1) Replace all existing [IndexedGetter] with [V8CustomIndexedGetter], and make it work. (You can land the patch first.)
(2) Replace most of the [V8CustomIndexedGetter]s with [IndexedGetter]s by auto-generating the code. (You can do it incrementally.)

As we've discussed, ultimately, only NPObject and DOMWindow should have [V8CustomIndexedGetter] (with custom bindings), and other interfaces should have [IndexedGetter] (without custom bindings).

[Kentaro Hara]

Comment on attachment 190462 [details]
Patch

Let me mark r- for now due to the above comment.

[foo]

Created attachment 190676 [details]
Patch

[Kentaro Hara]

Comment on attachment 190676 [details]
Patch

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

Looks reasonable.

> Source/WebCore/ChangeLog:12
> +        This is preparation for refactoring CodeGeneratorV8.pm

You might want to elaborate on why you are making this change so that others can understand what we're doing.

[Kentaro Hara]

Comment on attachment 190676 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [WIP] [V8] Added V8CustomIndexedGetter to IDLs that correspond to existing custom indexedPropertyGetter.

Remove [WIP].

[foo]

Created attachment 190874 [details]
Patch

[foo]

Comment on attachment 190874 [details]
Patch

Updated Changelog.

[Kentaro Hara]

Comment on attachment 190874 [details]
Patch

ok, thanks!

[WebKit Review Bot]

Comment on attachment 190874 [details]
Patch

Clearing flags on attachment: 190874

Committed r144419: <http://trac.webkit.org/changeset/144419>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.