Re: [heycam/webidl] Replace serializers by toJSON and [Default] extended attribute (#323)

domenic commented on this pull request.

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

> +<h5 id="idl-tojson-operation">toJSON</h5>
+
+By declaring a “toJSON” [=regular operation=],
+an [=interface=] specifies how to convert the objects that implement it to [=JSON types=].
+
+The “toJSON” [=regular operation=] is reserved for this usage.
+It must take zero arguments and return a [=JSON type=].
+
+The <dfn id="dfn-json-types" export lt="JSON type">JSON types</dfn> are:
+
+*   [=numeric types=],
+*   {{boolean}},
+*   [=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"

> +
+The “toJSON” [=regular operation=] is reserved for this usage.
+It must take zero arguments and return a [=JSON type=].
+
+The <dfn id="dfn-json-types" export lt="JSON type">JSON types</dfn> are:
+
+*   [=numeric types=],
+*   {{boolean}},
+*   [=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=],
+*   [=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.

> +this is done by exposing a “toJSON” method
+which returns the [=JSON type=]
+converted into an ECMAScript value
+that can be turned into a JSON string
+by the <code>[=JSON.stringify=]</code> function.
+Additionaly, in the ECMAScript language binding,
+the “toJSON” operation can take a [{{Default}}] [=extended attribute=],
+in which case the [=default toJSON operation=] is exposed instead.
+
+<div class="example">
+
+    The following [=IDL fragment=] defines an interface <code class="idl">Transaction</code>
+    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.

> +*   [=union types=] where all of their [=member types=] are [=JSON types=],
+*   [=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=],
+*   [=interface types=] that have a “toJSON” operation declared on themselves or
+    one of their [=inherited interface|inherited=] or
+    [=consequential interface|consequential=] interfaces.
+
+How the “toJSON” [=regular operation=]
+is made available on an object in a language binding,
+and how exactly the [=JSON types=] are converted into a JSON string,
+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.

> +
+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">
+
+    As an example, the [{{Default}}] extended attribute is suitable
+    for use on the “toJSON” [=regular operation=] of the
+    <a href="https://w3c.github.io/browser-payment-api/#paymentresponse-interface"><code class="idl">PaymentResponse</code></a>
+    interface [[payment-request]].
+
+    <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?

> @@ -10684,11 +10463,19 @@ property-installation style as namespaces.)
                 [=identifier=] |id| on |target| and with argument count |n|.
             1.  Let &lt;|operation|, |values|&gt; be the result of passing
                 |S| and |arg|<sub>0..|n|−1</sub> to the [=overload resolution algorithm=].
-            1.  Let |R| be the result of performing the actions listed in the
-                description of |operation|, on |O| if |O| is not <emu-val>null</emu-val>, with |values|
-                as the argument values.
-            1.  Return the result of [=converted to an ECMAScript value|converting=] |R| to
-                an ECMAScript value of the type |op| is declared to return.
+            1.  Let |R| be <emu-val>null</emu-val>.
+            1.  If |operation| is declared with a [{{Default}}] [=extended attribute=],
+                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)

> +
+The “toJSON” [=regular operation=] is reserved for this usage.
+It must take zero arguments and return a [=JSON type=].
+
+The <dfn id="dfn-json-types" export lt="JSON type">JSON types</dfn> are:
+
+*   [=numeric types=],
+*   {{boolean}},
+*   [=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=],
+*   [=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=],

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

> +    1.  Invoke [=collect attribute values=], passing |stack| and |map|.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).
+    1.  [=map/For each=] |key| → |value| of |map|,
+        1.  Let |k| be |key| [=converted to an ECMAScript value=].
+        1.  Let |v| be |value| [=converted to an ECMAScript value=].
+        1.  Perform [=!=] [=CreateDataProperty=](|result|, |k|, |v|).
+    1.  Return |result|.
+</div>
+
+<div algorithm>
+
+    To invoke the <dfn>collect attribute values</dfn> abstract operation
+    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"

> +        1.  Perform [=!=] [=CreateDataProperty=](|result|, |k|, |v|).
+    1.  Return |result|.
+</div>
+
+<div algorithm>
+
+    To invoke the <dfn>collect attribute values</dfn> abstract operation
+    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|.
+    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=].
+        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"

> +
+    To invoke the <dfn>collect attribute values</dfn> abstract operation
+    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|.
+    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=].
+        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|.
+    1.  If a “toJSON” operation with a [{{Default}}] [=extended attribute=] is declared on |I|, then
+        [=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.

> +        then invoke [=collect attribute values=], passing it |stack| and |map|.
+</div>
+
+<div algorithm>
+
+    To <dfn>create an inheritance stack</dfn> for [=interface=] |I|,
+    run the the following steps:
+
+    1.  Let |stack| be a new [=stack=].
+    1.  [=stack/Push=] |I| onto |stack|.
+    1.  [=iteration/While=] |I| [=interface/inherits=] from an [=interface=],
+        1.  Let |I| be that [=interface=].
+        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:

```webidl
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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/heycam/webidl/pull/323#pullrequestreview-37674877