Bug 74027

Summary: [V8] CodeGeneratorV8.pm does not correctly work with inherited HasIndexGetter
Product: WebKit Reporter: Erik Arvidsson <arv>
Component: DOMAssignee: Erik Arvidsson <arv>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, mihnea, senorblanco, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Erik Arvidsson 2011-12-07 14:21:40 PST
For example, DOMSettableTokenList extends DOMTokenList and DOMTokenList has an item method. We should not need to have a V8DOMSettableTokeListCustom class just to forward the item calls.
Comment 1 Erik Arvidsson 2011-12-12 14:17:38 PST
Created attachment 118844 [details]
Patch
Comment 2 Stephen White 2011-12-12 14:22:55 PST
Looks good, but I'll leave for someone more wise in the ways of V8 bindings.  You should also remove bindings/v8/custom/V8WebKitCSSFilterValueCustom.cpp, otherwise I think the linker will complain.
Comment 3 Erik Arvidsson 2011-12-12 14:24:59 PST
Thanks Steven, updating the patch. (The linker does not complain because the .h file still declares this function even though it is never used.)
Comment 4 Adam Barth 2011-12-12 14:27:15 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=118844&action=review

> Source/WebCore/bindings/scripts/CodeGenerator.pm:267
> +        foreach my $function (@{$interface->functions}) {
> +            if ($function->signature->name eq $functionName) {
> +                $indexer = $function->signature;
> +                return 'prune';

What about overloads?  Also, what does "prune" mean?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-2027
> -    # FIXME: The item() getter is not inherited from CSSValueList, seemingly due to the way
> -    # the CodeGenerator->AddMethodsConstantsAndAttributesFromParentClasses() method works,

Why isn't the right solution to change AddMethodsConstantsAndAttributesFromParentClasses to understand indexers?
Comment 5 Stephen White 2011-12-12 14:29:39 PST
(In reply to comment #3)
> Thanks Steven, updating the patch. (The linker does not complain because the .h file still declares this function even though it is never used.)

Actually, I think it doesn't complain because CSS_FILTERS is not enabled by default (yet).  If it was, I think you'd get a duplicate symbol.
Comment 6 Erik Arvidsson 2011-12-12 14:35:55 PST
(In reply to comment #4)
> View in context: https://bugs.webkit.org/attachment.cgi?id=118844&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGenerator.pm:267
> > +        foreach my $function (@{$interface->functions}) {
> > +            if ($function->signature->name eq $functionName) {
> > +                $indexer = $function->signature;
> > +                return 'prune';
> 
> What about overloads?

I'll check. I don't think we support overloads in indexed getters.

> Also, what does "prune" mean?

"prune" is the sentinel used to stop the iteration in ForAllParents().

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-2027
> > -    # FIXME: The item() getter is not inherited from CSSValueList, seemingly due to the way
> > -    # the CodeGenerator->AddMethodsConstantsAndAttributesFromParentClasses() method works,
> 
> Why isn't the right solution to change AddMethodsConstantsAndAttributesFromParentClasses to understand indexers?

I'll check if that would be cleaner. It is important that we do not add an "item" method to the DOMSettableToken.prototype.
Comment 7 Erik Arvidsson 2011-12-12 15:17:08 PST
Created attachment 118877 [details]
Patch
Comment 8 Erik Arvidsson 2011-12-12 15:19:28 PST
(In reply to comment #6)
> (In reply to comment #4)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118844&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGenerator.pm:267
> > > +        foreach my $function (@{$interface->functions}) {
> > > +            if ($function->signature->name eq $functionName) {
> > > +                $indexer = $function->signature;
> > > +                return 'prune';
> > 
> > What about overloads?

We don't support overloaded indexers or named property getters. There is currently no use of this in any of the idl files.

> > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-2027
> > > -    # FIXME: The item() getter is not inherited from CSSValueList, seemingly due to the way
> > > -    # the CodeGenerator->AddMethodsConstantsAndAttributesFromParentClasses() method works,
> > 
> > Why isn't the right solution to change AddMethodsConstantsAndAttributesFromParentClasses to understand indexers?
> 
> I'll check if that would be cleaner. It is important that we do not add an "item" method to the DOMSettableToken.prototype.

This could have been done by adding an indexer and namedPropertyGetter to the interface object but to me that seems less clean than this solution.
Comment 9 WebKit Review Bot 2011-12-12 17:34:31 PST
Comment on attachment 118877 [details]
Patch

Clearing flags on attachment: 118877

Committed r102646: <http://trac.webkit.org/changeset/102646>
Comment 10 WebKit Review Bot 2011-12-12 17:34:35 PST
All reviewed patches have been landed.  Closing bug.