Bug 167274 - [WebIDL] Add support for inherit serializer attribute
Summary: [WebIDL] Add support for inherit 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: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-20 18:08 PST by Joseph Pecoraro
Modified: 2017-01-30 23:21 PST (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (54.33 KB, patch)
2017-01-20 18:18 PST, Joseph Pecoraro
joepeck: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (54.81 KB, patch)
2017-01-28 09:34 PST, Joseph Pecoraro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-01-20 18:08:16 PST
Add support for inherit serializer attribute

Also extend support for detecting if attributes are serializable.

WebIDL Spec:
https://heycam.github.io/webidl/#idl-serializers

---

Resource Timing wants serializer { inherit }:
https://w3c.github.io/resource-timing/

> interface PerformanceResourceTiming : PerformanceEntry {
>     ...
>     serializer = {inherit, attribute};
> };

The Performance API also has non-simple attributes that can be serialized:

> interface Performance : EventTarget {
>    readonly attribute PerformanceNavigation navigation;
>    readonly attribute PerformanceTiming timing;
>    ...
>    serializer = {attribute};
> };
Comment 1 Joseph Pecoraro 2017-01-20 18:18:11 PST
Created attachment 299416 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2017-01-20 21:14:27 PST
Comment on attachment 299416 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/bindings/scripts/CodeGenerator.pm:941
> +    my $interface = GetInterfaceForAttribute($object, $currentInterface, $attribute);
> +    return 1 if $interface && $interface->serializable;

Seems EventHandler makes it down to here and we try to look up EventHandler type and fail. I can easily bail above, but maybe there is a better list of non-Interface built-in types.

> Source/WebCore/bindings/scripts/test/JS/JSTestSerialization.cpp:424
> +    auto sixthNodeAttributeValue = jsTestSerializationSixthNodeAttributeGetter(*state, *thisObject, throwScope);
> +    ASSERT(!throwScope.exception());
> +    result->putDirect(vm, Identifier::fromString(&vm, "sixthNodeAttribute"), sixthNodeAttributeValue);

r- For interface types that are serializable, I should be serializing them. So I should be doing a toJSON on thing, not putting the value directly.
Comment 3 Joseph Pecoraro 2017-01-28 09:34:44 PST
Created attachment 300039 [details]
[PATCH] Proposed Fix
Comment 4 Darin Adler 2017-01-28 23:28:47 PST
Comment on attachment 300039 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/ChangeLog:16
> +        serializing and attribute that is itself a serializable interface.

an attribute

> Source/WebCore/ChangeLog:29
> +        special strings. The spec does not accurately define where the
> +        special "attribute" keyword is allowed.

What does "accurately" mean here? Do you mean "precisely"?

> Source/WebCore/bindings/scripts/CodeGenerator.pm:945
> +    my $interface = GetInterfaceForAttribute($object, $currentInterface, $attribute);
> +    if ($interface && $interface->serializable) {
> +        die "Serializer for non-primitive types is not currently supported";
> +    }

Should this have a newline at the end of the die message? How useful is this error when it happens? Is it easy to understand what type we are talking about and what we did wrong?
Comment 5 Joseph Pecoraro 2017-01-29 00:14:43 PST
Comment on attachment 300039 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/ChangeLog:29
>> +        special "attribute" keyword is allowed.
> 
> What does "accurately" mean here? Do you mean "precisely"?

"precisely" is much better.

>> Source/WebCore/bindings/scripts/CodeGenerator.pm:945
>> +    }
> 
> Should this have a newline at the end of the die message? How useful is this error when it happens? Is it easy to understand what type we are talking about and what we did wrong?

It should have a newline.

Here we are bailing when you reach unimplemented functionality in our generator. Previously `inherit` was unsupported and just did the same thing `die "Serializer inherit keyword is not currently supported.";`. This patch gets inherit working for primitive types, but not Interfaces that are serializable.

When the time comes that someone needs serialization functionality for Interfaces:

    interface SerializableObject {
        attribute long timestamp;
        serializer = { attribute };
    };

    interface SomeInterface {
        attribute SerializableObject object;
        serializer = { attribute };
    };

Then they will need to extend our generator to support it, and that is what this die case if fore. No such case exists yet.

--

Thinking deeper, we probably need to group sequence/dictionary in with Interface and bail. Since none of these cases exist I didn't have a real JavaScript case to test with, but it looks like the generation would just put the object there instead of a serialized version of the object. I'll change this to be more conservative.
Comment 6 Joseph Pecoraro 2017-01-30 23:21:15 PST
<https://trac.webkit.org/changeset/211409>