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

bzbarsky requested changes on this pull request.

In general, this looks pretty reasonable...

> @@ -1985,6 +1989,122 @@ The following extended attributes are applicable to operation arguments:
         "void"
 </pre>
 
+<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".

> @@ -1985,6 +1989,122 @@ The following extended attributes are applicable to operation arguments:
         "void"
 </pre>
 
+<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=].
+
+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"?

> +The [=corresponding default operation=] of the “toJSON” operation is the [=default toJSON operation=].
+
+<div algorithm>
+
+    To invoke the <dfn export>default toJSON operation</dfn> of [=interface=] |I|,
+    run the the following steps:
+
+    1.  Let |O| be the <emu-val>this</emu-val> value.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).
+    1.  If there is an [=inherited interface=] of |I|
+        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.
+        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`?

> +The [=corresponding default operation=] of the “toJSON” operation is the [=default toJSON operation=].
+
+<div algorithm>
+
+    To invoke the <dfn export>default toJSON operation</dfn> of [=interface=] |I|,
+    run the the following steps:
+
+    1.  Let |O| be the <emu-val>this</emu-val> value.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).
+    1.  If there is an [=inherited interface=] of |I|
+        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.
+        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?

> +<h6 id="es-default-tojson">default toJSON operation</h5>
+
+The [=corresponding default operation=] of the “toJSON” operation is the [=default toJSON operation=].
+
+<div algorithm>
+
+    To invoke the <dfn export>default toJSON operation</dfn> of [=interface=] |I|,
+    run the the following steps:
+
+    1.  Let |O| be the <emu-val>this</emu-val> value.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).
+    1.  If there is an [=inherited interface=] of |I|
+        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?

> +
+Only [=regular operations=] which have a <dfn>corresponding default operation</dfn> defined below
+may be declared with a [{{Default}}] [=extended attribute=].
+
+
+<h6 id="es-default-tojson">default toJSON operation</h5>
+
+The [=corresponding default operation=] of the “toJSON” operation is the [=default toJSON operation=].
+
+<div algorithm>
+
+    To invoke the <dfn export>default toJSON operation</dfn> of [=interface=] |I|,
+    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?

> +
+    To invoke the <dfn export>default toJSON operation</dfn> of [=interface=] |I|,
+    run the the following steps:
+
+    1.  Let |O| be the <emu-val>this</emu-val> value.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).
+    1.  If there is an [=inherited interface=] of |I|
+        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.
+        1.  Set |result| to [=?=] [=Call=](|super|, |O|).
+    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?

> +    run the the following steps:
+
+    1.  Let |O| be the <emu-val>this</emu-val> value.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).
+    1.  If there is an [=inherited interface=] of |I|
+        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.
+        1.  Set |result| to [=?=] [=Call=](|super|, |O|).
+    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|).
+        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.

> +    To invoke the <dfn export>default toJSON operation</dfn> of [=interface=] |I|,
+    run the the following steps:
+
+    1.  Let |O| be the <emu-val>this</emu-val> value.
+    1.  Let |result| be [=!=] [=ObjectCreate=]([=%ObjectPrototype%=]).
+    1.  If there is an [=inherited interface=] of |I|
+        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.
+        1.  Set |result| to [=?=] [=Call=](|super|, |O|).
+    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|).
+        1.  Let |created| be [=!=] [=CreateDataProperty=](|result|, |id|, |value|).

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

-- 
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-24303934