Replace serializers by toJSON and [Default] extended attribute #323

tobie · 2017-02-27T23:10:19Z

Remove all serializers.
Add support for JSON stringification through the toJSON operation.
Add the [Default] extended attribute.
Add a default JSON operation.

Fixes #188.
Closes #259.
Fixes #260.
Fixes #262.
Fixes #370.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27428.

Note: I opted for a generic [Default] extended attribute combined with a default toJSON method instead of jsonifier;. This is because it's the only solution I find to be able to get rid of the abstract intermediary JSON formats that was previously defined, by only defining a default toJSON operation for the ES binding.

There's probably a few typos and things that could be phrased better, but I really wanted to get some feedback on the overall structure of the patch, so please bear with me while I fix these smaller things. :)

Preview | Diff

tobie · 2017-02-27T23:16:36Z

index.bs

+            the closest [=inherited interface=] of |I|
+            that declares a “toJSON” operation.
+        1.  Set |result| to [=?=] [=Call=](|super|, |O|).
+    1.  If Type(|result|) is not Object, return |result|.


This behavior allows to set a [Default] extended attribute on a toJSON operation even it has ancestors that set a custom one.

If the custom toJSON operation defined on one of the ancestors returns an object, we just add the interface's attributes to it. Otherwise, we return whatever was returned by the ancestor's toJSON method.

Happy to hear comments as to why this is a bad idea.

So here we're invoking the actual operation (as opposed to operation steps), right? But the default value of that operation, not whatever is on the prototype?

So here we're invoking the actual operation (as opposed to operation steps), right?

Yes. At least that's the intent.

But the default value of that operation, not whatever is on the prototype?

That's also the intent, yes.

I wonder whether this is observably different from invoking the operation steps... Do we use the |super| from O's relevant global? Or from the current global of the toJSON operation? That is, what is the current global while |super|'s steps are being performed? This may be observable with toJSON operations that return non-primitives...

Mmm. I assumed that's what you had to do if you wanted to handle custom toJSON operations in inherited interfaces, e.g.:

interface Parent { ParentJSON toJSON(); }; dictionary ParentJSON { DOMstring bar; }; interface Child { attribute DOMstring foo: [Default] toJSON(); };

But I guess you can invoke the steps of Parent.toJSON rather than the actual operation.

That's a bit over my head tbh.

tobie · 2017-02-27T23:23:01Z

index.bs

+*   a [=sequence type=] whose parameterized type is a [=JSON value=],
+*   a [=dictionary=] where all of its [=dictionary members|members=] are [=JSON values=],
+*   a [=record=] where all of its [=map/values=] are [=JSON values=], and
+*   an [=object type=],


I'm wondering if the object type needs further restrictions here (such as having all of its properties be JSON values or if we don't really care because JSON.stringify will deal with it anyhow).

tobie · 2017-02-27T23:25:39Z

index.bs

+            The “toJSON” operation of the <code class="idl">Transaction</code> interface
+            must follow these steps:
+
+            1.  Let |json| be a new <code class="idl">TransactionJSONValue</code> dictionary.


This screams for slots and a default behavior of slots and getters/setters.

bzbarsky

In general, this looks pretty reasonable...

bzbarsky · 2017-02-28T17:53:48Z

index.bs

+<h5 id="idl-tojson-operation">toJSON</h5>
+
+By declaring a “toJSON” [=regular operation=],
+an [=interface=] specify how to convert the objects that implements it to [=JSON values=].


"Specifies" and "implement".

bzbarsky · 2017-02-28T17:54:52Z

index.bs

+The “toJSON” [=regular operation=] is reserved for this usage.
+It must take zero arguments and return a [=JSON value=].
+
+The list of <dfn id="dfn-json-values" export lt="JSON value">JSON values</dfn> is as follows:


This is a list of types, not values. Should we be talking about "JSON types"?

So this is named after ECMA-404 which distinguishes between JSON text and JSON values (with the former being the stringified version of the latter). Deep-linking to ECMA-404 isn't possible, since the spec is a pdf, so I didn't.

I also considered calling this a type, but then I wasn't sure if we could define a type in prose which had restrictions the way this one does.

I'll go with whatever you suggest here.

I'm OK with JSON values. Then we should say something like "The list of types that JSON values can have is:"

bzbarsky · 2017-02-28T18:01:38Z

index.bs

+            the closest [=inherited interface=] of |I|
+            that declares a “toJSON” operation.
+        1.  Set |result| to [=?=] [=Call=](|super|, |O|).
+    1.  If Type(|result|) is not Object, return |result|.


This is not very compatible with declaring toJSON to return object. Should the default toJSON be required to return any?

If that's the behavior we want, than yes, absolutely.

bzbarsky · 2017-02-28T18:03:28Z

index.bs

+            the closest [=inherited interface=] of |I|
+            that declares a “toJSON” operation.
+        1.  Set |result| to [=?=] [=Call=](|super|, |O|).
+    1.  If Type(|result|) is not Object, return |result|.


So here we're invoking the actual operation (as opposed to operation steps), right? But the default value of that operation, not whatever is on the prototype?

bzbarsky · 2017-02-28T18:04:45Z

index.bs

+        that declares a “toJSON” operation, then
+        1.  Let |super| be the “toJSON” operation  of
+            the closest [=inherited interface=] of |I|
+            that declares a “toJSON” operation.


This all does not play nicely with mixins, in our current mixin model. The idea should be that you consider your inherited interfaces and all their mixins, right?

That's a good point.

I think I need to rethink some of this.

@bzbarsky: would you recommend doing this by climbing up the prototype chain in ES?

I would really prefer we didn't do that actually, because that's mutable and can lead to weird behavior...

Tentatively fixed. This was only missing consequential interfaces, right?

Yes, I think just adding the consequential interfaces is what's needed for this bit.

bzbarsky · 2017-02-28T18:05:32Z

index.bs

+    run the the following steps:
+
+    1.  Let |O| be the <emu-val>this</emu-val> value.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).


Shouldn't this move into a "if there is no inherited toJSON" branch?

The second one of those seems fine, with "let |result| be" or something.

Sounds good. Fixed the second example's copy/paste error.

bzbarsky · 2017-02-28T18:08:39Z

index.bs

+    1.  If Type(|result|) is not Object, return |result|.
+    1.  For each [=exposed=] [=regular attribute=] |attr| that is an [=interface member=] of |I|:
+        1.  Let |id| be |attr|’s [=identifier=] [=converted to an ECMAScript value=].
+        1.  Let |value| be [=?=] [=Get=](|O|, |id|).


So this is calling possibly-modified getters, not the original ones, right? Why?

This isn't by design. I just didn't think about it. I must confess having no idea how to express what you're suggesting we do here. Is the following what you have in mind? Is there a clearer way to spec this?

1. Let |value| be the result of invoking the [=Get=] operation of interface |I| passing it |O| and |id| as argument.

I'm not sure what the right way to phrase this in the current world is. Maybe @domenic knows offhand?

I haven't had time yet to review this PR (hopefully today), but the two proposed phrasings are identical. If I understand the context correctly, I think you want something like "Let value be the result of running the steps specified for the getter of the IDL attribute id on object O". But please read that skeptically as I am jumping into the middle here without much context.

The tricky bit, which is also the one I'm conceptually struggling the most with (if my understanding of it is correct), is that you want to be calling the getters of interface |I|, which is not necessarily the one that |O| implements. Consider the following example (@bzbarsky, stop me if I'm wrong):

interface Grandparent { [Default] any toJSON(); attribute DOMString x; // returns "gp-x" }; interface Parent : Grandparent { [Default] any toJSON(); attribute DOMString y; // returns "p-y" }; interface Child : Parent { attribute DOMString x; // returns "c-x" attribute DOMString z; // returns "c-z" };

new GrandParent().toJSON(); // { x: "gp-x" }; new Parent().toJSON(); // { x: "gp-x", y: "p-y" }; new Child().toJSON(); // { x: "gp-x", y: "p-y" };

In the last example, notice how we called Grandparent's getter for x, not Child's. Also notice how z isn't stringified as toJSON isn't declared on Child.

Overall I see how that makes sense from an implementor's perspective, but coming from dynamic languages, I find that super hard to grasp and reason about.

Oh, good catch. In general, what should happen if I subclass a thing with a [Default] toJSON? Seems to me like conceptually it shouldn't matter whether the subclassing is done via defining an interface inheriting from it or via a page just subclassing an interface via ES class syntax.

It seems like one fundamental question is whether toJSON should be getting properties or reading internal slots...

bzbarsky · 2017-02-28T18:12:08Z

index.bs

+        1.  Let |id| be |attr|’s [=identifier=] [=converted to an ECMAScript value=].
+        1.  Let |value| be [=?=] [=Get=](|O|, |id|).
+        1.  Let |created| be [=!=] [=CreateDataProperty=](|result|, |id|, |value|).
+        1.  Assert: |created| is <emu-val>true</emu-val>.


You can't assert anything of the sort. You're calling CreateDataProperty on some random object that might have come from anywhere. It can totally fail to define the property.

bzbarsky · 2017-02-28T18:12:35Z

index.bs

+    1.  For each [=exposed=] [=regular attribute=] |attr| that is an [=interface member=] of |I|:
+        1.  Let |id| be |attr|’s [=identifier=] [=converted to an ECMAScript value=].
+        1.  Let |value| be [=?=] [=Get=](|O|, |id|).
+        1.  Let |created| be [=!=] [=CreateDataProperty=](|result|, |id|, |value|).


You can't use ! here. You don't control result; it could just throw from [[DefineOwnProperty]].

domenic

Overall looking pretty great. I guess the two big outstanding questions to me are:

Whether we want the generic-ness of [Default] and default operations
Whether we want the default algorithm to perform all these observable-from-ES steps, or go straight to the backing stuff

domenic · 2017-03-04T00:48:27Z

index.bs

+an [=interface=] specifies how to convert the objects that implement it to [=JSON values=].
+
+The “toJSON” [=regular operation=] is reserved for this usage.
+It must take zero arguments and return a [=JSON value=].


I think the distinction here between values and types is a bit confusing. I think just renaming this to "JSON type" is the way to go. If you ever have to say "a value of a JSON type" then you can do so, but that should be rare...

JSON type it is.

domenic · 2017-03-04T00:49:02Z

index.bs

+*   a [=dictionary=] where all of its [=dictionary members|members=] are [=JSON values=],
+*   a [=record=] where all of its [=map/values=] are [=JSON values=], and
+*   an [=object type=],
+*   an [=interface type=] that has a “toJSON” method declared on itself or


s/method/operation, right?

domenic · 2017-03-04T00:50:26Z

index.bs

+    one of its [=inherited interface|inherited=] or
+    [=consequential interface|consequential=] interface.
+
+How the “toJSON” [=regular operation=]


I might just omit this paragraph (and the next one), as kind of the point here is that it behaves just like any other operation in terms of language bindings. The restrictions are there so that when you perform the usual exposure steps, the result is still usable by ECMAScript's JSON.stringify, but toJSON itself isn't really special, besides just having to follow some restrictions.

domenic · 2017-03-04T00:51:34Z

index.bs

+            must follow these steps:
+
+            1.  Let |json| be a new <code class="idl">TransactionJSONValue</code> dictionary.
+            1.  Set |json|[“from”] to this <code class="idl">Transaction</code>'s associated "from" value.


Can we make these quotes not be curly?

Man, I saw those elsewhere in the spec and duly copied them. Good thing we have a document convention section where all this stuff is clarified. :D

Yeah, the spec seems to really like using curly quotes for operation names, but for dictionary fields at least I assumed we were following the infra-style dictionaries-as-ordered-maps-with-string-keys and infra-strings don't use curly quotes.

In the future maybe it'd be cool to use <code> for operation names instead of curly quotes. shrug

Oh, right those are the infra ones. Sure.

domenic · 2017-03-04T00:54:22Z

index.bs

@@ -8058,6 +7765,79 @@ for an interface is to be implemented.
 </div>


+<h4 id="Default" extended-attribute lt="Default">[Default]</h4>


~~Do we anticipate using this super-general "default operations" framework for anything else in the future? I'd be tempted to just go with [DefaultToJSON] and make it all specific to ToJSON here...~~Consider this resolved

domenic · 2017-03-04T00:55:21Z

index.bs

+    <pre highlight="webidl">
+        [SecureContext]
+        interface PaymentResponse {
+          readonly attribute DOMString paymentRequestId;


Note that this is probably changing to an id member in payment request. Maybe using a made-up example is better.

On my second pass, yeah, we should probably get rid of this example. It seems redundant with the other example in the toJSON section... Maybe move that one here? Or split that one into two, so that the "In the ECMAScript language binding..." part goes here, and we link back to the previous example for the language-agnostic stage-setting?

Still need to address that.

Partially addressed this, still a bit more to fix.

domenic · 2017-03-04T00:56:39Z

index.bs

+                then:
+                1.  Set |R| be the result of performing the actions listed in
+                    |operation|'s [=corresponding default operation=]
+                    on |O| if |O| is not <emu-val>null</emu-val>,


I think we should assert O is not null here instead of including this clause.

Similarly if we go for un-genericifying this we can assert that values is zero-length.

We can still assert O is not null here, as [Default] is restricted to regular operations (not static operations)

domenic · 2017-03-04T00:57:04Z

index.bs

+may be declared with a [{{Default}}] [=extended attribute=].
+
+
+<h6 id="es-default-tojson">default toJSON operation</h5>


Capitalize "D"

domenic · 2017-03-04T01:02:07Z

index.bs

+        1.  Let |id| be |attr|’s [=identifier=] [=converted to an ECMAScript value=].
+        1.  Let |value| be [=?=] [=Get=](|O|, |id|).
+        1.  Perform [=?=] [=CreateDataProperty=](|result|, |id|, |value|).
+    1.  Return |result|.


So I guess we have open questions as to whether this algorithm should observably call the super, or observably perform Get()s. I think it's easy enough to phrase it as doing neither. E.g. to be non-observable instead of

Let result be ? Call(super, O)

say something like

Let result be the result of performing the steps listed in the description of super on O (or the default steps defined for super if super is annotated with [Default])

(and switch around subsequent the type testing to work on IDL values instead of ES values).

And instead of

Let value be ? Get(O, id)

say something like

Let value be the result of performing the steps listed in the description the getter for id on O.

tobie · 2017-03-20T17:57:40Z

Finally adressing @domenic's comments here. Sorry for the delay in getting back to this.

I guess the two big outstanding questions to me are:

Whether we want the generic-ness of [Default] and default operations

The reason behind this was that (1) it turned out to be trivial to generalize, (2) it was more aesthetically pleasing to write: [Default] any toJSON(); than [DefaultToJSON] any toJSON();, and (3) it was more extensible, thus more future proof. I didn't really see an issues with the generalized version besides a false promise that it could be used for other operations and a slightly higher cognitive load when reading the spec.

That said, I'm not attached to it at all and I'm happy to go with whatever people prefer.

Whether we want the default algorithm to perform all these observable-from-ES steps, or go straight to the backing stuff

I don't have a strong opinion here outside of saying that it is really hard to spec the ES route. As discussed above, ES Get relies on the prototype chain and, apparently, that's not what makes sense from an implementation perspective. We'd need to spec dedicated Get operations here that would call the getters defined higher up in the prototype chain. This means we'll need to track how ES Get is specified and edit our own version accordingly. I'd rather avoid that.

The whole situation feels somewhat of a code smell. While the [Default] toJSON operation certainly makes sense in C++, it feels awkward in JS, where you'd probably implement that dynamically. If we're nonetheless sure this is the right algorithm to spec, we should probably got straight to the backing stuff and replace that with slots once we have those spec'ed.

domenic · 2017-04-17T20:14:43Z

Re: [Default] vs. [DefaultToJSON]. I do not have strong opinions, and agree with your overview of the possible pros. I am just skeptical that we will ever use the generality. So I'm happy to leave it up to you; of course if @bzbarsky or anyone else has stronger opinions that would be good to hear.

BTW I am quite happy with the extended attribute approach these days; notably it fits with other cases where we use extended attributes to "generate" the definition of the operation/attribute, like [PutForwards].

Re: overall strategy. I think going straight to the backing data is the better way to go. It matches the current spec (see e.g. #188 (comment) discussion) and thus will result in no extra implementation churn.

* Remove all serializers. * Add support for JSON stringification through the toJSON operation. * Add the [Default] extended attribute. * Add a default JSON operation. Fixes whatwg#260. Fixes whatwg#188. Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27428.

* Uses underlying values instead of converted to ES ones. * Relies on the underlying abstract operation instead of the exposed JS method. * No longer relies on the result of custom toJSON methods in inherited interfaces. * Explicit about how consequential and inherited interfaces are handled.

This is possible now that we no longer take into consideration custom toJSON operations in the [Default] algorithm.

tobie · 2017-05-11T13:08:49Z

Cleaned-up the algorithm to rely on underlying value and abstract operations instead of ES Get and calling the toJSON method of objects. Note that with this new design, I can switch to using dedicated syntax instead (aka jsonifier;) if that's preferred to using [Default].
Being very explicit about order of inherited and consequential interfaces. I hope that's both readable and correct.
Rebased the whole thing and adjusted accordingly (hopefully) to deal with type extended attributes.
Haven't simplified the examples yet. Maybe that would be a useful thing to do before merging.
Decided to keep [Default] instead of [DefaultToJSON] as:
1. Even if we never have another use for [Default], it still reads better than [DefaultToJSON], so I'd like to settle on it whether or nor we keep the infrastructure that allows to generalize it.
2. The infrastructure to generalize the use case is not exposed, so we can always remove it later.
3. Some of the stuff discussed around arrays might call for some other use of [Default] (maybe?).
4. I'm lazy.

tobie · 2017-05-11T13:24:44Z

@domenic this should be ready for another round of review right now.

domenic

Looking quite good!! Just some minor stuff to finish up now.

domenic · 2017-05-11T19:20:08Z

index.bs

+*   [=string types=],
+*   [=nullable types=] whose [=nullable types/inner type=] is a [=JSON type=],
+*   [=annotated types=] whose [=annotated types/inner type=] is a [=JSON type=],
+*   [=union types=] where all of their [=member types=] are [=JSON types=],


Nit: you switched from "whose" to "where". I think all "whose" might work well, e.g. "union types whose member types are all JSON types"

domenic · 2017-05-11T19:21:15Z

index.bs

+*   [=sequence types=] whose parameterized type is a [=JSON type=],
+*   [=dictionaries=] where all of their [=dictionary members|members=] are [=JSON types=],
+*   [=records=] where all of their [=map/values=] are [=JSON types=],
+*   [=object types=],


object types doesn't seem correct; e.g. it includes interfaces, or object could be used to return literally anything. I think just omitting this is OK; if someone really can't come up with a dictionary or record for their return type, we can address that in the future.

Hmm, I guess we need object for the default case. So maybe this should just be object, not object types? We could also prohibit object unless you use [Default].

Initially confused [=object=] with [=object types=]. Now fixed to include only the former.

Should be code-ified like boolean is

domenic · 2017-05-11T19:22:24Z

index.bs

+    that has a “toJSON” method defined in prose,
+    and an interface <code class="idl">Account</code>,
+    which relies on the [=default toJSON operation=]
+    (its “toJSON” operation is declared with a [{{Default}}] [=extended attribute=]):


I'd get rid of the parenthetical; it's repetitive given you already stated this equivalence one paragraph ago.

domenic · 2017-05-11T19:23:09Z

index.bs

+is language binding specific.
+
+Note: In the ECMAScript language binding,
+this is done by exposing a “toJSON” method


I assume we're not consistent here, but maybe methods should be <code>toJSON</code>, even if we keep the ugly convention of curly-quotes for operation identifiers? Not a big deal.

domenic · 2017-05-11T19:27:18Z

index.bs

+    <pre highlight="webidl">
+        [SecureContext]
+        interface PaymentResponse {
+          readonly attribute DOMString paymentRequestId;


On my second pass, yeah, we should probably get rid of this example. It seems redundant with the other example in the toJSON section... Maybe move that one here? Or split that one into two, so that the "In the ECMAScript language binding..." part goes here, and we link back to the previous example for the language-agnostic stage-setting?

domenic · 2017-05-11T19:29:14Z

index.bs

+                then:
+                1.  Set |R| be the result of performing the actions listed in
+                    |operation|'s [=corresponding default operation=]
+                    on |O| if |O| is not <emu-val>null</emu-val>,


We can still assert O is not null here, as [Default] is restricted to regular operations (not static operations)

domenic · 2017-05-11T19:32:05Z

index.bs

+    with [=stack=] |stack| and [=ordered map=] |map| as arguments,
+    run the the following steps:
+
+    1.  Let |I| be the result of invoking [=stack/pop=] from |stack|.


"the result of popping from stack"

domenic · 2017-05-11T19:32:34Z

index.bs

+        1.  Let |rhs| be the [=interface=] represented by the right-hand side [=identifier=].
+        1.  Let |interfaces| be the result of [=create an inheritance stack|creating an inheritance stack=]
+            for [=interface=] |rhs|.
+        1.  invoke [=collect attribute values=], passing it |interfaces| and |map|.


Uppercase "invoke"

domenic · 2017-05-11T19:35:50Z

index.bs

+        [=list|for each=] [=exposed=] [=regular attribute=] |attr|
+        that is an [=interface member=] of |I|, in order:
+        1.  Let |id| be the [=identifier=] of |attr|.
+        1.  Let |value| be the underlying value of |attr|.


I think this is better stated as "Let value be the result of performing the actions listed in the description of attr that occur on getting (or those listed in the description of the inherited attribute, if this attribute is declared to inherit its getter)".

This phrase appears in #dfn-attribute-getter, and maybe should be deduplicated.

Would adding something like the below in §2.2.2 Attributes do?

To get the value of an [=attribute=] |attribute| declared in interface |I|, implemented by object |O|, run the following steps: 1. Let |R| be the result of performing the actions listed in the description of |attribute| that occur on getting (or those listed in the description of the inherited attribute, if this attribute is declared to [=inherit its getter=]), on |O| if |O| is not <emu-val>null</emu-val>. 1. Return |R|.

How would you handle the "on |O| if |O| is not null" part keeping it in WebIDL, though?

It looks like in the latest spec revision includes two steps for getting value, one old and one new; so we should fix that.

As for the abstraction: I don't think there's a need for separate let/return steps, or for the interface I. So I'd phrase this as:

To get the underlying value of an attribute attr given a value target, return the result of performing the actions listed in the description of attr that occur on getting, or those listed in the description of the inherited attribute, if attr is declared to inherit its getter, on target if target is not null.

But we could leave that for later.

A preference (wrapping the whole thing in div.algorithm vs. marking variables as ignorable in Bikeshed)?

<div algorithm> To <dfn export>get the underlying value</dfn> of an attribute |attr| given a value |target|, return the result of performing the actions listed in the description of |attr| that occur on getting, or those listed in the description of the inherited attribute, if |attr| is declared to inherit its getter, on |target| if |target| is not null. </div>

or:

To <dfn export>get the underlying value</dfn> of an attribute <var ignore>attr</var>given a value <var ignore>target</var>, return the result of performing the actions listed in the description of <var ignore>attr</var> that occur on getting, or those listed in the description of the inherited attribute, if <var ignore>attr</var> is declared to inherit its getter, on <var ignore>target</var> if <var ignore>target</var> is not null.

Algorithm seems good.

domenic · 2017-05-11T19:41:18Z

index.bs

+        1.  [=stack/Push=] |I| onto |stack|.
+    1.  Return |stack|.
+</div>
+


This whole section would, of course, benefit from an example. I think it should be short and contrived but illustrate the algorithm. Something like:

interface A : B { attribute x; }; interface B { attribute y; }; interface C { attribute z; }; interface D { attribute w; }; A implements C; B implements D;

and then explain what keys the map ends up containing, and in what order.

tobie · 2017-05-15T16:23:31Z

Would need your input on #323 (comment) (surfacing it as GH's helpful UI hid it somewhere).

Still need to address removing some of the extra JSON examples.

Otherwise, I think I addressed everything in your latest feedback.

domenic · 2017-05-15T23:34:32Z

index.bs

+<div class=example>
+
+    The following [=IDL fragment=] defines a number of [=interfaces=]
+    which are [=inherited interface|inherited=] or [=consequential interfaces=] of `A`,


Backtick <code>ification doesn't seem to have worked for some reason

I get burnt by that everytime. :(

domenic · 2017-05-15T23:34:43Z

index.bs

+        G* - A - H*
+    </pre>
+
+    Interfaces markes with an asterixk ("*")


domenic · 2017-05-15T23:35:05Z

index.bs

+    </pre>
+
+    Calling the <code>toJSON</code> method of an object
+    implementing interface `A` defined above


<code> instead of backticks

domenic · 2017-05-15T23:40:26Z

index.bs

+        "s": "...",
+        "x": "...",
+        "y": "..."
+    }


OK, so it would be easier to check this if the lowercase property names matched the uppercase interface names.

So, this order is a bit surprising to me. I would the following interface order:

C, B, F, E, I, D, A, G, H

My reasoning is:

First go to the most distance ancestor on the JavaScript prototype chain (C).

Then go to the next-most-distance, B

Then go B's mixins

But D has inheritance, so go up its "chain", ending up with F; then E plus its I mixin; then finally D

Now we can do A, and then its two mixins, G and H.

This all might be moot once we simplify interfaces... but I think it'd be good to get right, if possible. We could add a warning that we anticipate such complex inherited/consequential interface chains not being possible in the future, which would make all this less ridiculous. Probably linking to the mixin issue I'm about to open.

So we mostly agree, except you consider mixins happen after interface definition rather than before.

interface A { attribute DOMString a; }; interface B { attribute DOMString b; }; A implements B;

Do we get {a,b} or {b,a}?

I don't think the spec is clear about that, tbh.

Yeah, not clear as-is. I think it should definitely be {a,b} though, as the "main" stuff takes priority, and stuff added by other specs would come later, not first.

Rely on get the underlying value dfn.

tobie · 2017-05-17T15:29:30Z

It feels sort of icky that the whole default operation is ES-only, especially since the boundary between the IDL part and the ES binding is now so clear.

It seems other extended attributes are similarly not ES-specific (e.g. [PutForwards]).

Should we not worry about this until WASM is a thing?

domenic · 2017-05-17T18:01:16Z

I was going make a few more tweaks, but it appears "enable edits from collaborators" is not enabled, so here's a diff:

diff --git a/index.bs b/index.bs
index 3757492..2fd5d64 100644
--- a/index.bs
+++ b/index.bs
@@ -2026,7 +2026,7 @@ Additionaly, in the ECMAScript language binding,
 the <code>toJSON</code> operation can take a [{{Default}}] [=extended attribute=],
 in which case the [=default toJSON operation=] is exposed instead.
 
-<div class="example">
+<div class="example" id="tojson-example">
 
     The following [=IDL fragment=] defines an interface <code class="idl">Transaction</code>
     that has a <code>toJSON</code> method defined in prose:
@@ -2055,18 +2055,18 @@ in which case the [=default toJSON operation=] is exposed instead.
 
     <div algorithm="example tojson">
 
-        To invoke the <code>toJSON</code> operation of the <code class="idl">Transaction</code> interface
-        on |target|, run the following steps:
+        To invoke the <code>toJSON()</code> operation of the <code class="idl">Transaction</code>
+        interface, run the following steps:
 
         1.  Let |json| be a new <code class="idl">TransactionJSONValue</code> dictionary.
-        1.  [=list/For each=] attribute [=identifier=] |attr| in [=list=] « "from", "to", "amount", "description" »:
+        1.  [=list/For each=] attribute [=identifier=] |attr| in « "from", "to", "amount", "description" »:
             1.  Let |value| be result of [=get the underlying value|getting the underlying value=]
-                of [=attribute=] |attr| of <code class="idl">Transaction</code> on |target|.
+                of the [=attribute=] identified by |attr|, given |target|.
             1.  Set |json|[|attr|] to |value|.
         1.  Return |json|.
     </div>
 
-    In the ECMAScript language binding, there would exist a <code>toJSON</code> method on
+    In the ECMAScript language binding, there would exist a <code>toJSON()</code> method on
     <code class="idl">Transaction</code> objects:
 
     <pre highlight="js">
@@ -7897,7 +7897,7 @@ The [{{Default}}] extended attribute must not
 be used on anything other than a [=regular operation=]
 for which a [=corresponding default operation=] has been defined.
 
-<div class="example">
+<div class="example" id="example-tojson-default">
 
     As an example, the [{{Default}}] extended attribute is suitable
     for use on the <code>toJSON</code> [=regular operation=] of the
@@ -10355,7 +10355,7 @@ The characteristics of this property are as follows:
                         [=extended attribute=], then return <emu-val>undefined</emu-val>.
                     1. Otherwise, [=ECMAScript/throw=] a <emu-val>TypeError</emu-val>.
             1.  Let |R| be the result of [=get the underlying value|getting the underlying value=]
-                of |attribute| on |O|.
+                of |attribute| given |O|.
             1.  Return the result of [=converted to an ECMAScript value|converting=] |R| to an
                 ECMAScript value of the type |attribute| is declared as.
 
@@ -10609,7 +10609,7 @@ The [=return type=] of the [=default toJSON operation=] must be {{object}}.
         that is an [=interface member=] of |I|, in order:
         1.  Let |id| be the [=identifier=] of |attr|.
         1.  Let |value| be the result of [=get the underlying value|getting the underlying value=]
-            of |attr| for |O|.
+            of |attr| given |O|.
         1.  If |value| is a [=JSON type=], then [=map/set=] |map|[|id|] to |value|.
     1.  [=list|For each=] [=implements statements=] where the left-hand side [=identifier=] references |I|:
         1.  Let |rhs| be the [=interface=] represented by the right-hand side [=identifier=].
@@ -10635,7 +10635,7 @@ The [=return type=] of the [=default toJSON operation=] must be {{object}}.
     1.  Return |stack|.
 </div>
 
-<div class=example>
+<div class=example id=example-tojson-default-inheritance-and-mixins>
 
     The following [=IDL fragment=] defines a number of [=interfaces=]
     which are [=inherited interface|inherited=] or [=consequential interfaces=] of <code class="idl">A</code>,
@@ -10719,7 +10719,7 @@ The [=return type=] of the [=default toJSON operation=] must be {{object}}.
     }
     </pre>
 
-    An object implementing interface `B` would return:
+    An object implementing interface <code class="idl">B</code> would return:
 
     <pre highlight=json>
     {

It feels sort of icky that the whole default operation is ES-only, especially since the boundary between the IDL part and the ES binding is now so clear.

It seems fine to me. All extended attributes, including [PutForwards], are ES-specific; they're in the ECMAScript binding section. I agree there's some confusing bleed-over, e.g. the interfaces section says which extended attributes are applicable to interfaces; maybe that should be consolidated into the extended attribute definitions instead. But yeah, not worth worrying about for now.

Still would be good to replace the PaymentRequest example with a made-up one.

tobie · 2017-05-17T19:34:22Z

I was going make a few more tweaks, but it appears "enable edits from collaborators" is not enabled, so here's a diff.

Thanks. Not sure what happened there.

Still would be good to replace the PaymentRequest example with a made-up one.

Yes. Absolutely.

tobie · 2017-05-18T17:07:48Z

Fixed the last example with something that made more sense.

Last nit: there's little consistency with toJSON() vs toJSON (no parens). @domenic: LMK what your reasoning behind this is and I'll fix it.

domenic · 2017-05-18T19:35:08Z

Last nit: there's little consistency with toJSON() vs toJSON (no parens). @domenic: LMK what your reasoning behind this is and I'll fix it.

It seems OK to me as-is. My take is that:

It's important to use parens when declaring the steps for an operation, so that if there are arguments you can include those too (and then refer to them in the steps)
It's generally nice to include the parens when talking about a method or operation
It's better to not include the parens when talking about an identifier.

The remaining question then is when you say something like "the toJSON regular operation" are you using toJSON as an identifier or as a method name. I could go either way. It seems like the current state of this PR is to treat it as an identifier, which works fine for me.

One minor diff, but LGTM to merge otherwise. Let's do this!!

diff --git a/index.bs b/index.bs
index 24edade..b68344a 100644
--- a/index.bs
+++ b/index.bs
@@ -7922,8 +7922,8 @@ for which a [=corresponding default operation=] has been defined.
 
     In the ECMAScript language binding, there would exist a <code>toJSON()</code> method on
     <code class="idl">Animal</code>, <code class="idl">Human</code>,
-    and <code class="idl">Dog</code> objects:
-    
+    and (via inheritance) <code class="idl">Dog</code> objects:
+
     <pre highlight="js">
         // Get an instance of Human.
         var alice = getHuman();
@@ -7937,7 +7937,7 @@ for which a [=corresponding default operation=] has been defined.
         //   pet: Dog
         // }
         alice.toJSON();
-        
+
         // Evaluates to an object like this (notice how "breed" is absent,
         // as the Dog interface doesn't declare a default toJSON operation):
         //
@@ -10701,7 +10701,7 @@ The [=return type=] of the [=default toJSON operation=] must be {{object}}.
         E implements I;
     </pre>
 
-    Calling the <code>toJSON</code> method of an object
+    Calling the <code>toJSON()</code> method of an object
     implementing interface <code class="idl">A</code> defined above
     would return the following JSON object:

tobie · 2017-05-18T19:41:00Z

How did the "Allow edits from maintainers" option get unchecked again?!

domenic · 2017-05-18T19:46:06Z

The bug seems to be that it gets unchecked every push :-/.

tobie · 2017-05-18T19:49:05Z

I just checked it again, it doesn't even hold. As soon as the xhr/fetch request is done, the input gets unchecked again. Filing an issue with GitHub.

See whatwg/webidl#323.

…ttribute 他 Add bug template URL for Chromium ( whatwg/webidl#374 ) whatwg/webidl@4269cc0 40ac0890 Add frozen array types to the list of JSON types whatwg/webidl@375b588 27d62c6c Editorial: minor language fix whatwg/webidl@d42f606 a840517b Editorial: fix toJSON algorithm example whatwg/webidl@b323f1a e62d9718 Replace serializers by toJSON and [Default] extended attribute ( whatwg/webidl#323 ) whatwg/webidl@a505f33 7f0877b1

bzbarsky · 2018-02-07T13:09:04Z

@tobie Did implementation bugs ever get filed on the changes here? There were substantive changes to the list of serializable types too (like adding records), right?

tobie · 2018-02-07T13:43:17Z

@bzbarsky: yes, tracked from right here: #372

bzbarsky · 2018-02-07T15:48:30Z

Thank you. I do recall seeing that now, but it didn't mention the changes to the list of serializable types, which affected the priority... :(

tobie · 2018-02-07T16:01:36Z

I'm sorry, I didn't think about this when filing it. I'll try to make sure not to forget small fixes done in passing next time I file vendor bugs.

tobie commented Feb 27, 2017

View reviewed changes

bzbarsky requested changes Feb 28, 2017

View reviewed changes

domenic reviewed Mar 4, 2017

View reviewed changes

tobie mentioned this pull request Mar 20, 2017

Define initializing variables and setting them? whatwg/infra#88

Closed

tobie mentioned this pull request May 2, 2017

There are too many bot comments web-platform-tests/wpt#5611

Closed

tobie added 9 commits May 11, 2017 14:21

Partially address bz's feedback.

7b9cd65

Include toJSON operations declared on consequential interfaces.

7a6522b

JSON values includes interfaces that inherited their toJSON op.

8c29b0d

Respect alphabetical order.

dc35b5d

JSON value -> JSON type

ea40489

Remove curly quotes.

df463f0

Proper capitalization.

88f7004

tobie force-pushed the tojson branch from fe1f6e4 to 88c1793 Compare May 11, 2017 12:23

tobie added 3 commits May 11, 2017 14:24

Handle both inner types in default toJSON operations

68b333a

Change return type of default toJSON to object.

1ad7cff

This is possible now that we no longer take into consideration custom toJSON operations in the [Default] algorithm.

Add missing var in abstract operation argument list.

a85daf3

domenic reviewed May 11, 2017

View reviewed changes

tobie added 2 commits May 15, 2017 16:51

Partially address feedback.

7300610

Re-include object that was mistakenly removed before.

d979a3f

domenic reviewed May 15, 2017

View reviewed changes

tobie added 4 commits May 16, 2017 02:23

Match the attribute identifier to its interface

26094a1

*cough*

0765d96

Handle mixing after interface's own members.

f252e51

Simplify first toJSON example.

5c1699d

Rely on get the underlying value dfn.

Applying @domenic's patch.

a693c16

Improve [Default] toJSON example.

ea59256

Last fixes (hopefully).

e2a1a4e

tobie force-pushed the tojson branch from 8d70bd7 to e2a1a4e Compare May 19, 2017 06:57

tobie mentioned this pull request Jun 14, 2017

Default to json operation web-platform-tests/wpt#6246

Merged

domenic mentioned this pull request Jun 17, 2017

Serializable types should include unrestricted double and unrestricted float #370

Closed

tobie merged commit a505f33 into whatwg:master Jun 21, 2017

tobie mentioned this pull request Jun 21, 2017

Track PR #323 #372

Closed

28 tasks

tobie deleted the tojson branch June 22, 2017 00:01

This was referenced Jun 23, 2017

Remove serializer-related productions w3c/webidl2.js#73

Closed

Remove serializer-related productions plinss/widlparser#25

Closed

tobie added a commit to web-platform-tests/wpt that referenced this pull request Jun 23, 2017

[idlharness] Add support for testing toJSON operations (#6246)

f94a32c

See whatwg/webidl#323.

littledan mentioned this pull request Dec 4, 2018

How should the toJSON method be used? w3ctag/design-principles#116

Closed

		@@ -8058,6 +7765,79 @@ for an interface is to be implemented.
		</div>


		<h4 id="Default" extended-attribute lt="Default">[Default]</h4>

		may be declared with a [{{Default}}] [=extended attribute=].


		<h6 id="es-default-tojson">default toJSON operation</h5>

Replace serializers by toJSON and [Default] extended attribute #323

Replace serializers by toJSON and [Default] extended attribute #323

Conversation

tobie commented Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Mar 20, 2017 • edited Loading

domenic commented Apr 17, 2017

tobie commented May 11, 2017 • edited Loading

tobie commented May 11, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Feb 27, 2017 •

edited

Loading

tobie Feb 27, 2017 •

edited

Loading

tobie Feb 28, 2017 •

edited

Loading

tobie Mar 2, 2017 •

edited

Loading

tobie Mar 2, 2017 •

edited

Loading

domenic Mar 4, 2017 •

edited

Loading

tobie commented Mar 20, 2017 •

edited

Loading

tobie commented May 11, 2017 •

edited

Loading