Bug 163466 – Implement serializer = { attribute }

• Component: Bindings
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

I'm trying to use serializer = { attribute }; in bug 163464 but the object gets no serialization attributes.

[Simon Fraser (smfr)]

It also generates code that doesn't build: bug 163464.

[Simon Fraser (smfr)]

Darin and I wrote some code for this.

[Simon Fraser (smfr)]

Created attachment 291707 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 291709 [details]
Patch

[Alejandro G. Castro]

Thanks for implementing the new feature. I think you probably want to modify the parser to handle this case, and generate the list of serializable attributes, I'm not sure actually what you get in the list of attributes in this case because the keyword is not supported, probably none. In the parseSerializationAttributes function add a new if for this case.

[Alejandro G. Castro]

Comment on attachment 291709 [details]
Patch

I don't think this solution is correct, as I explained you need to modify the parser and check the list of serializable types from the list of all the attributes to generate the correct list of attributes, instead of trying to detect them in the generator: https://heycam.github.io/webidl/#dfn-serializable-type. In the applyMemberList function you can check the serializable after parsing the keyword correctly and just add the ones that comply with the serializable types.

[Simon Fraser (smfr)]

Created attachment 291723 [details]
Patch

[Darin Adler]

I don’t think the parser should implement the concept of "serializable attributes"; the parser doesn’t know enough about all the types to know which ones should be treated as serializable.

[Darin Adler]

Comment on attachment 291723 [details]
Patch

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

> Source/WebCore/bindings/scripts/IDLParser.pm:2340
> +sub isSerializableAttribute
> +{
> +    my $attribute = shift;
> +
> +    # FIXME: Need to support more than primitive serializable types.
> +    my $serializable_types = '^(\(byte|octet|short|unsigned short|long|unsigned long|long long|unsigned long long|float|unrestricted float|double|unrestricted double|boolean|DOMString|ByteString|USVString)$';
> +    return $attribute->signature->type =~ /$serializable_types/;
> +}

I am not sure that the concept is whether an attribute is serializable belongs in the parser.

[Darin Adler]

(In reply to comment #5)
> I'm not sure actually what you get in the list of attributes in
> this case because the keyword is not supported, probably none.

You get the attribute name "attribute"; we tested the original patch and it worked.

[Simon Fraser (smfr)]

Comment on attachment 291723 [details]
Patch

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

>> Source/WebCore/bindings/scripts/IDLParser.pm:2340
>> +}
> 
> I am not sure that the concept is whether an attribute is serializable belongs in the parser.

I'll adjust the FIXME accordingly.

[Simon Fraser (smfr)]

https://trac.webkit.org/r207378

[Darin Adler]

Is the serializer line allowed to be before the interface? If not, do we need to check for that case and report an error?

[youenn fablet]

Comment on attachment 291723 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3988
> +            if ($attribute_name eq $attribute->signature->name) {

If serialiser uses an attribute not defined by the interface, we will silently skip it. Might be better to die I guess, except if WebIDL spec says so.
Maybe the parser should give a list of attributes and not a list of attribute names.

> Source/WebCore/bindings/scripts/IDLParser.pm:1199
> +        $domSerializable->hasGetter(1);

It might have been good to not die in the parser but die in the binding generator since it is a binding generator limitation.
Also this would allow writing
hasGetter(1) if...

>>> Source/WebCore/bindings/scripts/IDLParser.pm:2340
>>> +}
>> 
>> I am not sure that the concept is whether an attribute is serializable belongs in the parser.
> 
> I'll adjust the FIXME accordingly.

There is potentially the need to know all IDL types as an attribute mat return a value whose type is defined by an interface marked as serializable.
The parser might not have that knowledge.

[Darin Adler]

Comment on attachment 291723 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3988
>> +            if ($attribute_name eq $attribute->signature->name) {
> 
> If serialiser uses an attribute not defined by the interface, we will silently skip it. Might be better to die I guess, except if WebIDL spec says so.
> Maybe the parser should give a list of attributes and not a list of attribute names.

Yes, this behavior is bad. I don’t think it’s important *where* the check is, but it’s valuable to check if the attribute name doesn’t match any attribute and cause an error.

>> Source/WebCore/bindings/scripts/IDLParser.pm:1199
>> +        $domSerializable->hasGetter(1);
> 
> It might have been good to not die in the parser but die in the binding generator since it is a binding generator limitation.
> Also this would allow writing
> hasGetter(1) if...

Sure, slightly better, but there is no practical difference -- either way the fix is to update the IDL file -- so I wouldn’t waste time worrying about where the check is.

>>>> Source/WebCore/bindings/scripts/IDLParser.pm:2340
>>>> +}
>>> 
>>> I am not sure that the concept is whether an attribute is serializable belongs in the parser.
>> 
>> I'll adjust the FIXME accordingly.
> 
> There is potentially the need to know all IDL types as an attribute mat return a value whose type is defined by an interface marked as serializable.
> The parser might not have that knowledge.

Yes, that was what I meant. Anyway, no immediate crisis here, but that thinking was one reason I put the handling of "attribute" in the code generator in my first cut at this.

[Simon Fraser (smfr)]

(In reply to comment #15)
> Comment on attachment 291723 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291723&action=review
> 
> >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3988
> >> +            if ($attribute_name eq $attribute->signature->name) {
> > 
> > If serialiser uses an attribute not defined by the interface, we will silently skip it. Might be better to die I guess, except if WebIDL spec says so.
> > Maybe the parser should give a list of attributes and not a list of attribute names.
> 
> Yes, this behavior is bad. I don’t think it’s important *where* the check
> is, but it’s valuable to check if the attribute name doesn’t match any
> attribute and cause an error.

Fixed in https://trac.webkit.org/r207394

[Darin Adler]

(In reply to comment #16)
> Fixed in https://trac.webkit.org/r207394

That has the word "serializate" in it!

[Alejandro G. Castro]

I agree with Darin, not sure about the proper place of the attributes serializable detection, my point was really about the parser support for the situation. Great patch, thanks for adding it.

[Darin Adler]

Another tiny improvement we could make would be to have some kind of error at compile time if there is a list with no attribute names in it. Currently, instead of failing to generate code, we will generate code that won’t compile in that case.

[Simon Fraser (smfr)]

I fixed the typo. I will leave other cleanup for bindings gurus :)