Bug 74027 – [V8] CodeGeneratorV8.pm does not correctly work with inherited HasIndexGetter

• Component: DOM
• Status: RESOLVED FIXED

[Erik Arvidsson]

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.

[Erik Arvidsson]

Created attachment 118844 [details]
Patch

[Stephen White]

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.

[Erik Arvidsson]

Thanks Steven, updating the patch. (The linker does not complain because the .h file still declares this function even though it is never used.)

[Adam Barth]

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?

[Stephen White]

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

[Erik Arvidsson]

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

[Erik Arvidsson]

Created attachment 118877 [details]
Patch

[Erik Arvidsson]

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

[WebKit Review Bot]

Comment on attachment 118877 [details]
Patch

Clearing flags on attachment: 118877

Committed r102646: <http://trac.webkit.org/changeset/102646>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.