Bug 163466 - Implement serializer = { attribute }
Summary: Implement serializer = { attribute }
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks: 163473
  Show dependency treegraph
 
Reported: 2016-10-14 16:02 PDT by Simon Fraser (smfr)
Modified: 2016-10-17 11:28 PDT (History)
10 users (show)

See Also:


Attachments
Patch (50.28 KB, patch)
2016-10-14 22:28 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (51.24 KB, patch)
2016-10-14 22:45 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (58.56 KB, patch)
2016-10-15 13:16 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-10-14 16:02:07 PDT
I'm trying to use serializer = { attribute }; in bug 163464 but the object gets no serialization attributes.
Comment 1 Simon Fraser (smfr) 2016-10-14 16:51:53 PDT
It also generates code that doesn't build: bug 163464.
Comment 2 Simon Fraser (smfr) 2016-10-14 18:29:18 PDT
Darin and I wrote some code for this.
Comment 3 Simon Fraser (smfr) 2016-10-14 22:28:31 PDT
Created attachment 291707 [details]
Patch
Comment 4 Simon Fraser (smfr) 2016-10-14 22:45:48 PDT
Created attachment 291709 [details]
Patch
Comment 5 Alejandro G. Castro 2016-10-14 23:36:17 PDT
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 6 Alejandro G. Castro 2016-10-14 23:57:15 PDT
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.
Comment 7 Simon Fraser (smfr) 2016-10-15 13:16:13 PDT
Created attachment 291723 [details]
Patch
Comment 8 Darin Adler 2016-10-15 14:07:53 PDT
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 9 Darin Adler 2016-10-15 14:09:21 PDT
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.
Comment 10 Darin Adler 2016-10-15 14:12:38 PDT
(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 11 Simon Fraser (smfr) 2016-10-15 14:13:35 PDT
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.
Comment 12 Simon Fraser (smfr) 2016-10-15 14:18:23 PDT
https://trac.webkit.org/r207378
Comment 13 Darin Adler 2016-10-15 15:50:47 PDT
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 14 youenn fablet 2016-10-16 07:35:15 PDT
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 15 Darin Adler 2016-10-16 08:38:53 PDT
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.
Comment 16 Simon Fraser (smfr) 2016-10-16 12:02:38 PDT
(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
Comment 17 Darin Adler 2016-10-16 17:10:28 PDT
(In reply to comment #16)
> Fixed in https://trac.webkit.org/r207394

That has the word "serializate" in it!
Comment 18 Alejandro G. Castro 2016-10-16 23:04:04 PDT
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.
Comment 19 Darin Adler 2016-10-17 09:23:23 PDT
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.
Comment 20 Simon Fraser (smfr) 2016-10-17 11:28:48 PDT
I fixed the typo. I will leave other cleanup for bindings gurus :)