Bug 163466

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

Darin and I wrote some code for this.

Created attachment 291707 [details] Patch

Created attachment 291709 [details] Patch

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.

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.

Created attachment 291723 [details] Patch

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.

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.

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

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.

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

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.

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.

(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

(In reply to comment #16) > Fixed in https://trac.webkit.org/r207394 That has the word "serializate" in it!

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.

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.

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