Bug 97335 – DOM4: Add support for rest parameters to DOMTokenList

• Component: DOM
• Status: RESOLVED FIXED

[Erik Arvidsson]

DOM4 allows passing multiple arguments to DOMTokenList add and remove.

[Erik Arvidsson]

*** Bug 97343 has been marked as a duplicate of this bug. ***

[Erik Arvidsson]

Discussion: https://www.w3.org/Bugs/Public/show_bug.cgi?id=13999

[Erik Arvidsson]

Created attachment 165803 [details]
WIP

[Erik Arvidsson]

The DOMTokenList.* files are not really done at this point.

At this point I want feedback on the generated code. Does this approach look reasonable for variadic arguments?

[Adam Barth]

> At this point I want feedback on the generated code. Does this approach look reasonable for variadic arguments?

It looks plausible to me, but this is really a question for haraken.

[Ojan Vafai]

Comment on attachment 165803 [details]
WIP

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

> Source/WebCore/html/DOMTokenList.h:53
>      virtual void add(const AtomicString&, ExceptionCode&) = 0;
>      virtual void remove(const AtomicString&, ExceptionCode&) = 0;

Just curious, with the new code, do these ever get called or do we always call the Vector versions? It's hard to tell since I'm unfamiliar with the bindings code. If the latter, can we delete these two methods or are they called internal to WebCore?

[Erik Arvidsson]

(In reply to comment #6)
> (From update of attachment 165803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165803&action=review
> 
> > Source/WebCore/html/DOMTokenList.h:53
> >      virtual void add(const AtomicString&, ExceptionCode&) = 0;
> >      virtual void remove(const AtomicString&, ExceptionCode&) = 0;
> 
> Just curious, with the new code, do these ever get called or do we always call the Vector versions? It's hard to tell since I'm unfamiliar with the bindings code. If the latter, can we delete these two methods or are they called internal to WebCore?

I'm working on these now.

The DOM bindings will never call these but there is other WebCore code that calls them. I might just update the call sites.

[Erik Arvidsson]

Created attachment 165863 [details]
Patch

[Erik Arvidsson]

Created attachment 165868 [details]
Patch

[Early Warning System Bot]

Comment on attachment 165868 [details]
Patch

Attachment 165868 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14021630

[Early Warning System Bot]

Comment on attachment 165868 [details]
Patch

Attachment 165868 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14043260

[Erik Arvidsson]

Created attachment 165876 [details]
Patch

[Kentaro Hara]

Comment on attachment 165876 [details]
Patch

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

The binding part and test cases LGTM with some comments.

ojan: would you take a look at the WebCore/html part?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2720
> +            if ($argType eq "DOMString") {
> +                $nativeElementType = "String";

Why can't you use GetNativeType() for DOMString?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2724
> +                    $nativeElementType = $nativeElementType . " ";

Nit: $nativeElementType .= " ";

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1423
> +    my $hasVariadic = 0;
>      my $numMandatoryParams = @{$function->parameters};
>      foreach my $parameter (@{$function->parameters}) {
>          if ($parameter->extendedAttributes->{"Optional"}) {
>              push(@orExpression, GenerateParametersCheckExpression($numParameters, $function));
>              $numMandatoryParams--;
>          }
> +        if ($parameter->isVariadic) {
> +            $hasVariadic = 1;
> +            last;
> +        }
>          $numParameters++;
>      }
> -    push(@orExpression, GenerateParametersCheckExpression($numParameters, $function));
> +    if (!$hasVariadic) {
> +        push(@orExpression, GenerateParametersCheckExpression($numParameters, $function));
> +    }

This looks correct, but shall we make the logic the same as the CodeGeneratorJS.pm one?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1773
> +                $nativeElementType = $nativeElementType . " ";

Nit: $nativeElementType .= " ";

> Source/WebCore/bindings/scripts/IDLParser.pm:251
> +        my $isVariadic = $paramTypeSuffix eq "...";

tasak@ is about to land the new IDLParser.pm in a day (https://bugs.webkit.org/show_bug.cgi?id=26398). I'd be happy if you could modify the new IDLParser.

[Erik Arvidsson]

Comment on attachment 165876 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2720
>> +                $nativeElementType = "String";
> 
> Why can't you use GetNativeType() for DOMString?

GetNativeType returns "const String&"

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1423
>> +    }
> 
> This looks correct, but shall we make the logic the same as the CodeGeneratorJS.pm one?

Done.

>> Source/WebCore/bindings/scripts/IDLParser.pm:251
>> +        my $isVariadic = $paramTypeSuffix eq "...";
> 
> tasak@ is about to land the new IDLParser.pm in a day (https://bugs.webkit.org/show_bug.cgi?id=26398). I'd be happy if you could modify the new IDLParser.

I see that it landed overnight. I'll rebase.

[Erik Arvidsson]

Created attachment 166000 [details]
Patch

[Kentaro Hara]

Comment on attachment 166000 [details]
Patch

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

r+ for the V8 binding part and test cases. Please r+ it if the WebCore/html part is OK.

> Source/WebCore/bindings/scripts/IDLParser.pm:1249
> -        $self->parseEllipsis();
> +        $paramDataNode->isVariadic($self->parseEllipsis());

Great! The new IDL parser is much much easier to modify:)

> Source/WebCore/bindings/scripts/IDLStructure.pm:115
>  our $typeNamespaceSelector = '((?:' . $idlId . '*::)*)\s*(' . $idlDataType . '*)';
>  
> -our $interfaceSelector = '(interface|exception)\s*((?:' . $extendedAttributeSyntax . ' )?)(' . $idlIdNs . '*)\s*(?::(\s*[^{]*))?{([-a-zA-Z0-9_"=\s(),;:\[\]<>&\|?]*)';
> -our $interfaceMethodSelector = '\s*((?:' . $extendedAttributeSyntax . ' )?)(static\s+)?' . $supportedTypes . '\s*(' . $idlIdNs . '*)\s*\(\s*([a-zA-Z0-9:\s,=\[\]<>?]*)';
> +our $interfaceSelector = '(interface|exception)\s*((?:' . $extendedAttributeSyntax . ' )?)(' . $idlIdNs . '*)\s*(?::(\s*[^{]*))?{([-a-zA-Z0-9_"=\s(),;:\[\]<>&\|?.]*)';
> +our $interfaceMethodSelector = '\s*((?:' . $extendedAttributeSyntax . ' )?)(static\s+)?' . $supportedTypes . '\s*(' . $idlIdNs . '*)\s*\(\s*([a-zA-Z0-9:\s,=\[\]<>?.]*)';
>  our $interfaceParameterSelector = '(in|out)\s*((?:' . $extendedAttributeSyntax . ' )?)' . $supportedTypes . $supportedTypeSuffix . '\s*(' . $idlIdNs . '*)';
>  
>  our $interfaceAttributeSelector = '\s*(static\s+)?(readonly attribute|attribute)\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)';

I think that these variables are no longer used by the IDL parser. I'll remove them in a follow-up patch.

[Erik Arvidsson]

(In reply to comment #16)
> > Source/WebCore/bindings/scripts/IDLStructure.pm:115
> >  our $typeNamespaceSelector = '((?:' . $idlId . '*::)*)\s*(' . $idlDataType . '*)';
> >  
> > -our $interfaceSelector = '(interface|exception)\s*((?:' . $extendedAttributeSyntax . ' )?)(' . $idlIdNs . '*)\s*(?::(\s*[^{]*))?{([-a-zA-Z0-9_"=\s(),;:\[\]<>&\|?]*)';
> > -our $interfaceMethodSelector = '\s*((?:' . $extendedAttributeSyntax . ' )?)(static\s+)?' . $supportedTypes . '\s*(' . $idlIdNs . '*)\s*\(\s*([a-zA-Z0-9:\s,=\[\]<>?]*)';
> > +our $interfaceSelector = '(interface|exception)\s*((?:' . $extendedAttributeSyntax . ' )?)(' . $idlIdNs . '*)\s*(?::(\s*[^{]*))?{([-a-zA-Z0-9_"=\s(),;:\[\]<>&\|?.]*)';
> > +our $interfaceMethodSelector = '\s*((?:' . $extendedAttributeSyntax . ' )?)(static\s+)?' . $supportedTypes . '\s*(' . $idlIdNs . '*)\s*\(\s*([a-zA-Z0-9:\s,=\[\]<>?.]*)';
> >  our $interfaceParameterSelector = '(in|out)\s*((?:' . $extendedAttributeSyntax . ' )?)' . $supportedTypes . $supportedTypeSuffix . '\s*(' . $idlIdNs . '*)';
> >  
> >  our $interfaceAttributeSelector = '\s*(static\s+)?(readonly attribute|attribute)\s*(' . $extendedAttributeSyntax . ' )?' . $supportedTypes . '\s*(' . $idlType . '*)';
> 
> I think that these variables are no longer used by the IDL parser. I'll remove them in a follow-up patch.

Yeah, my patch works when I remove all of these

[Erik Arvidsson]

Created attachment 166017 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166017 [details]
Patch

Clearing flags on attachment: 166017

Committed r129779: <http://trac.webkit.org/changeset/129779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I think we should investigate having the variadic string support use Vector<AtomicString> for the same reason that the single-argument functions use AtomicString.

[Erik Arvidsson]

(In reply to comment #21)
> I think we should investigate having the variadic string support use Vector<AtomicString> for the same reason that the single-argument functions use AtomicString.

Agreed. I filed bug 97911

[Alexey Shvayka]

*** Bug 122528 has been marked as a duplicate of this bug. ***