Bug 110852 - [V8] Added V8CustomIndexedGetter to IDLs that correspond to existing custom indexedPropertyGetters.
Summary: [V8] Added V8CustomIndexedGetter to IDLs that correspond to existing custom i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: foo
URL:
Keywords:
Depends on:
Blocks: 111810
  Show dependency treegraph
 
Reported: 2013-02-25 23:54 PST by foo
Modified: 2013-03-07 19:47 PST (History)
10 users (show)

See Also:


Attachments
Patch (6.28 KB, patch)
2013-02-26 00:05 PST, foo
no flags Details | Formatted Diff | Diff
Patch (5.72 KB, patch)
2013-02-27 00:58 PST, foo
no flags Details | Formatted Diff | Diff
Patch (6.78 KB, patch)
2013-02-28 01:30 PST, foo
no flags Details | Formatted Diff | Diff
Patch (7.09 KB, patch)
2013-02-28 20:35 PST, foo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description foo 2013-02-25 23:54:28 PST
[WIP] [V8] Generate IndexGetter bindings
Comment 1 foo 2013-02-26 00:05:21 PST
Created attachment 190218 [details]
Patch
Comment 2 Kentaro Hara 2013-02-26 01:04:21 PST
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.
Comment 3 foo 2013-02-27 00:58:17 PST
Created attachment 190462 [details]
Patch
Comment 4 Kentaro Hara 2013-02-27 01:13:59 PST
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).
Comment 5 Kentaro Hara 2013-02-27 01:16:05 PST
Comment on attachment 190462 [details]
Patch

Let me mark r- for now due to the above comment.
Comment 6 foo 2013-02-28 01:30:50 PST
Created attachment 190676 [details]
Patch
Comment 7 Kentaro Hara 2013-02-28 01:35:20 PST
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.
Comment 8 Kentaro Hara 2013-02-28 01:35:44 PST
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].
Comment 9 foo 2013-02-28 20:35:12 PST
Created attachment 190874 [details]
Patch
Comment 10 foo 2013-02-28 20:35:52 PST
Comment on attachment 190874 [details]
Patch

Updated Changelog.
Comment 11 Kentaro Hara 2013-02-28 22:15:04 PST
Comment on attachment 190874 [details]
Patch

ok, thanks!
Comment 12 WebKit Review Bot 2013-02-28 22:26:14 PST
Comment on attachment 190874 [details]
Patch

Clearing flags on attachment: 190874

Committed r144419: <http://trac.webkit.org/changeset/144419>
Comment 13 WebKit Review Bot 2013-02-28 22:26:18 PST
All reviewed patches have been landed.  Closing bug.