Summary: | [V8] Added V8CustomIndexedGetter to IDLs that correspond to existing custom indexedPropertyGetters. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | foo <kojih> | ||||||||||
Component: | WebCore JavaScript | Assignee: | foo <kojih> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, cmarcelo, esprehn+autocc, haraken, japhet, mifenton, nbarth, ojan.autocc, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 111810 | ||||||||||||
Attachments: |
|
Description
foo
2013-02-25 23:54:28 PST
Created attachment 190218 [details]
Patch
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. Created attachment 190462 [details]
Patch
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 on attachment 190462 [details]
Patch
Let me mark r- for now due to the above comment.
Created attachment 190676 [details]
Patch
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 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]. Created attachment 190874 [details]
Patch
Comment on attachment 190874 [details]
Patch
Updated Changelog.
Comment on attachment 190874 [details]
Patch
ok, thanks!
Comment on attachment 190874 [details] Patch Clearing flags on attachment: 190874 Committed r144419: <http://trac.webkit.org/changeset/144419> All reviewed patches have been landed. Closing bug. |