Re: [w3c/push-api] Migrate the serializer to a toJSON method. (#267)

tobie approved this pull request.

So this is the first conversion from a custom serializer in the wild, so bear with me and please feel free to ask for clarification if any of my comments require it.

Overall this looks good.

Added a couple of nits and proposals to shorten the prose using [infra](https://infra.spec.whatwg.org/) language which WebIDL now normatively references.

>          <a>user agent</a> MUST use a serialization method that does not contain input-dependent
         branchs (that is, one that is constant time). Note that a URL - as ASCII text - will not
         ordinarily require special treatment.
         </li>
-        <li>Add an entry to <var>map</var> whose key name is <code>expirationTime</code> and whose
-        value is the result of converting the <code>expirationTime</code> attribute to a serialized
-        value.
-        </li>
-        <li>Let <var>keys</var> be an empty map.
+        <li>Set the <code>expirationTime</code> property of <var>json</var> to the result of
+        converting the <a data-link-for="PushSubscription">expirationTime</a> attribute to a

I'd change that to something like the below:

```
Set the <code>expirationTime</code> property of <var>json</var>
to the result of <a data-link-for="get the underlying value">getting the underlying value</a>
of the <a data-link-for="PushSubscription">expirationTime</a> attribute,
given this PushSubscription object.
```

_(Note this references https://heycam.github.io/webidl/#get-the-underlying-value.)_

Additionally, you can use the <var>json</var>["expirationTime"] notation like so:

```
Set <var>json</var>["expirationTime"] to the result of […]
```
 

>        </ol>
+
+      <p>
+        A <dfn>PushSubscriptionJSON</dfn> dictionary represents the <a>JSON type</a> of a
+        <a>PushSubscription</a> that can be converted into a JSON string.
+      </p>
+      <p>
+        The <dfn data-dfn-for="PushSubscriptionJSON">endpoint</dfn> contains the serialized value of

I'm uneasy about using the serialized value term, as it's not defined anywhere. I'd use the "underlying value" here and again reference https://heycam.github.io/webidl/#get-the-underlying-value.

> @@ -896,7 +898,14 @@
           [SameObject] readonly attribute PushSubscriptionOptions options;
           ArrayBuffer? getKey(PushEncryptionKeyName name);
           Promise&lt;boolean&gt; unsubscribe();
-          serializer;
+
+          PushSubscriptionJSON toJSON();
+        };
+
+        dictionary PushSubscriptionJSON {
+          USVString endpoint;
+          DOMTimeStamp? expirationTime;
+          record keys;

So this should probably be `record<USVString, USVString>` (or `DOMString`?—I must admit still being unsure when to use which.)

>          <a>user agent</a> MUST use a serialization method that does not contain input-dependent
         branchs (that is, one that is constant time). Note that a URL - as ASCII text - will not
         ordinarily require special treatment.
         </li>
-        <li>Add an entry to <var>map</var> whose key name is <code>expirationTime</code> and whose
-        value is the result of converting the <code>expirationTime</code> attribute to a serialized
-        value.
-        </li>
-        <li>Let <var>keys</var> be an empty map.
+        <li>Set the <code>expirationTime</code> property of <var>json</var> to the result of
+        converting the <a data-link-for="PushSubscription">expirationTime</a> attribute to a
+        serialized value.
         </li>

Here's I write something like this:

```
Let <var>keys</var> be a new empty instance of record<USVString, USVString>.
```


> @@ -1009,20 +1016,36 @@
             <var>b</var> as a <a>USVString</a>. The <a>user agent</a> MUST use a serialization
             method that does not branch based on the value of <var>b</var>.
             </li>
-            <li>Add an entry to <var>keys</var> whose key name is the name of <var>i</var> and
-            whose value is <var>s</var>.
+            <li>Add an entry to the <a data-link-for="PushSubscriptionJSON">keys</a> property of

And here:

```
Set <var>keys</var>[<var>i</var>] to <var>s</var>.
```

> @@ -1009,20 +1016,36 @@
             <var>b</var> as a <a>USVString</a>. The <a>user agent</a> MUST use a serialization
             method that does not branch based on the value of <var>b</var>.
             </li>
-            <li>Add an entry to <var>keys</var> whose key name is the name of <var>i</var> and
-            whose value is <var>s</var>.
+            <li>Add an entry to the <a data-link-for="PushSubscriptionJSON">keys</a> property of
+            <var>json</var> whose key is the name of <var>i</var> and whose value is <var>s</var>.
             </li>
           </ol>
         </li>

And finally:

```
Set <var>json</var>["keys"] to <var>keys</var>.
```

>        </ol>
+
+      <p>
+        A <dfn>PushSubscriptionJSON</dfn> dictionary represents the <a>JSON type</a> of a
+        <a>PushSubscription</a> that can be converted into a JSON string.
+      </p>
+      <p>
+        The <dfn data-dfn-for="PushSubscriptionJSON">endpoint</dfn> contains the serialized value of
+        the <a data-link-for="PushSubscription">endpoint</a> attribute.
+      </p>
+      <p>
+        The <dfn data-dfn-for="PushSubscriptionJSON">expirationTime</dfn> contains the serialized
+        value of the <a data-link-for="PushSubscription">expirationTime</a> attribute.
+      </p>
+      <p>
+        The <dfn data-dfn-for="PushSubscriptionJSON">keys</dfn> contains a mapping from each

I'd replace the above by:

```
The <dfn data-dfn-for="PushSubscriptionJSON">keys</dfn> record contains an entry for each
```
(see https://infra.spec.whatwg.org/#map-entry if you want to reference the term properly.)

>        </ol>
+
+      <p>
+        A <dfn>PushSubscriptionJSON</dfn> dictionary represents the <a>JSON type</a> of a
+        <a>PushSubscription</a> that can be converted into a JSON string.

(and eventually append: "by JSON.stringify.") 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/push-api/pull/267#pullrequestreview-45754589