Re: [whatwg/encoding] Make TextEncoder and TextDecoder be transform streams (#127)

annevk commented on this pull request.

Mostly style nits (note that I've only reported the first instance).

I was actually hoping IDL's algorithm could become https://infra.spec.whatwg.org/#javascript-string-convert, but I couldn't really find a way to reuse that here so we might be stuck with some UTF-16 logic in multiple places forever.

> @@ -28,6 +28,24 @@ Translate IDs: dictdef-textdecoderoptions textdecoderoptions,dictdef-textdecodeo
 spec:infra; type:dfn;
     text:code point
     text:ascii case-insensitive
+spec:streams; type:interface;
+    text:ReadableStream
+    text:WritableStream
+</pre>
+
+<pre class=anchors>
+spec: streams; urlPrefix: https://streams.spec.whatwg.org/
+    type:dfn; text:chunk; url:#chunk
+    type:dfn; text:readable stream; url:#readable-stream
+    type:dfn; text:writable stream; url:#writable-stream
+    type:abstract-op; text:CreateTransformStream; url: #create-transform-stream
+    type:abstract-op; text:TransformStreamDefaultControllerEnqueue; url: #transform-stream-default-controller-enqueue
+    type:interface; text:TransformStream; url: #ts-class

What's not exported yet in streams should be exported so there's no need to list it here.

> +    text:WritableStream
+</pre>
+
+<pre class=anchors>
+spec: streams; urlPrefix: https://streams.spec.whatwg.org/
+    type:dfn; text:chunk; url:#chunk
+    type:dfn; text:readable stream; url:#readable-stream
+    type:dfn; text:writable stream; url:#writable-stream
+    type:abstract-op; text:CreateTransformStream; url: #create-transform-stream
+    type:abstract-op; text:TransformStreamDefaultControllerEnqueue; url: #transform-stream-default-controller-enqueue
+    type:interface; text:TransformStream; url: #ts-class
+spec: ECMASCRIPT; urlPrefix: https://tc39.github.io/ecma262/
+    text: type; url: #sec-ecmascript-data-types-and-values; type: dfn
+    text: IsDetachedBuffer; url: #sec-isdetachedbuffer; type: abstract-op
+    text: IsSharedArrayBuffer; url: #sec-issharedarraybuffer; type: abstract-op
+    text: internal slot; url: #sec-object-internal-methods-and-internal-slots; type: dfn

We generally don't cross-reference "internal slot".

> @@ -1186,10 +1246,31 @@ if <a for=TextDecoder>error mode</a> is "<code>fatal</code>", and false otherwis
 <p>The <dfn attribute for=TextDecoder><code>ignoreBOM</code></dfn> attribute's getter must return
 true if <a for=TextDecoder>ignore BOM flag</a> is set, and false otherwise.
 
+<p>The <dfn attribute for=TextDecoder><code>readable</code></dfn> attribute's getter must return the
+contents of <a for=TextDecoder>transform</a>'s \[[readable]] <a>internal slot</a>.

I think rather than "the contents of x" we usually say "x's value". @domenic? 

>  <p>The <dfn method for=TextDecoder><code>decode(<var>input</var>, <var>options</var>)</code></dfn>
 method, when invoked, must run these steps:
 
 <ol>
+ <li><p>Let <var>readable</var> be <a for=TextDecoder>transform</a>'s \[[readable]] <a>internal
+ slot</a>.
+
+ <li><p>If <a abstract-op>IsReadableStreamLocked</a>(<var>readable</var>) is true, throw a
+ {{TypeError}} exception.

then throw* (xref throw?)

> @@ -1241,6 +1322,88 @@ method, when invoked, must run these steps:
   </ol>
 </ol>
 
+<p>The <dfn id=concept-td-decode-and-enqueue>decode and enqueue a chunk</dfn> algorithm, given a
+{{TextDecoder}} <var>dec</var> and a <var>chunk</var>, runs these steps:
+
+<ol>
+ <li><p>If the <a>type</a> of of <var>chunk</var> is not Object, or <var>chunk</var> does not have

Shouldn't this be noted as Type(chunk)?

> @@ -1241,6 +1322,88 @@ method, when invoked, must run these steps:
   </ol>
 </ol>
 
+<p>The <dfn id=concept-td-decode-and-enqueue>decode and enqueue a chunk</dfn> algorithm, given a
+{{TextDecoder}} <var>dec</var> and a <var>chunk</var>, runs these steps:
+
+<ol>
+ <li><p>If the <a>type</a> of of <var>chunk</var> is not Object, or <var>chunk</var> does not have
+ an \[[ArrayBufferData]] <a>internal slot</a>, or <a
+ abstract-op>IsDetachedBuffer</a>(<var>chunk</var>) is true, or <a
+ abstract-op>IsSharedArrayBuffer</a>(<var>chunk</var>) is true then return <a>a promise rejected

Comma before then. Also I'm not sure I want to reference the promises guide as it really needs to be folded into IDL and has a bunch of things that confuse folks. Can we just say "a new promise rejected with a TypeError exception"?

> @@ -1241,6 +1322,88 @@ method, when invoked, must run these steps:
   </ol>
 </ol>
 
+<p>The <dfn id=concept-td-decode-and-enqueue>decode and enqueue a chunk</dfn> algorithm, given a
+{{TextDecoder}} <var>dec</var> and a <var>chunk</var>, runs these steps:
+
+<ol>
+ <li><p>If the <a>type</a> of of <var>chunk</var> is not Object, or <var>chunk</var> does not have
+ an \[[ArrayBufferData]] <a>internal slot</a>, or <a
+ abstract-op>IsDetachedBuffer</a>(<var>chunk</var>) is true, or <a
+ abstract-op>IsSharedArrayBuffer</a>(<var>chunk</var>) is true then return <a>a promise rejected
+ with</a> a {{TypeError}}.
+
+ <li><p>If the <a for=TextDecoder>do not flush flag</a> for <var>dec</var> is unset, set

then set*

> + <var>dec's</var> <a for=TextDecoder>decoder</a> to a new <a for=/>decoder</a> for <var>dec</var>'s
+ <a for=TextDecoder>encoding</a>, set <var>dec</var>'s <a for=TextDecoder>stream</a> to a new <a
+ for=/>stream</a>, and unset <var>dec</var>'s <a for=TextDecoder>BOM seen flag</a>.
+
+ <li><p>Set <var>dec</var>'s <a for=TextDecoder>do not flush flag</a>.
+
+ <li><p><a>Push</a> a <a lt="get a copy of the buffer source">copy of</a> <var>chunk</var> to
+ <var>dec</var>'s <a for=TextDecoder>stream</a>.
+
+ <li><p>Let <var>controller</var> be <var>dec</var>'s <a for=TextDecoder>transform</a>'s
+ \[[transformStreamController]] <a>internal slot</a>.
+
+ <li><p>Let <var>output</var> be a new <a for=/>stream</a>.
+
+ <li>
+  <p>While true, run these substeps:

steps* we try to avoid "substeps" and friends these days.

> + <var>output</var>, and <var>dec</var>'s <a for=TextDecoder>error mode</a>.
+
+ <li><p>If <var>result</var> is <a>finished</a>, run these substeps:
+ <ol>
+  <li><p>Let <var>outputChunk</var> be <var>output</var>, <a lt="serialize stream">serialized</a>.
+
+  <li><p>Let <var>controller</var> be <var>dec</var>'s <a for=TextDecoder>transform</a>'s
+  \[[transformStreamController]] <a>internal slot</a>.
+
+  <li><p>Call <a abstract-op>TransformStreamDefaultControllerEnqueue</a>(<var>controller</var>, <var>outputChunk</var>).
+
+  <li><p>Return <a>a promise resolved with</a> undefined.
+ </ol>
+
+ <li><p>Otherwise, return <a>a promise rejected with</a> a {{TypeError}}.
+</ol>

It seems like you ate a newline here.

> @@ -1267,6 +1434,23 @@ requires buffering of scalar values.
  <dt><code><var>encoder</var> . <a attribute for=TextEncoder>encoding</a></code>
  <dd><p>Returns "<code>utf-8</code>".
 
+ <dt><code><var>encoder</var> . <a attribute for=TextEncoder>readable</a></code>
+ <dd>
+  <p>Returns a <a>readable stream</a> whose <a>chunks</a> are {{Uint8Array}}s resulting from running
+  <a>UTF-8</a>'s <a for=/>encoder</a> on the chunks written to {{TextEncoder/writable}}.
+
+ <dt><code><var>encoder</var> . <a attribute for=TextEncoder>writable</a></code>
+ <dd>
+  <p>Returns a <a>writable stream</a> which accepts string chunks and runs them through
+  <a>UTF-8</a>'s <a for=/>encoder</a> before making them available to {{TextEncoder/readable}}.
+
+  <p>Typically this will be used via the {{ReadableStream/pipeThrough()}} method on a {{ReadableStream}} source.
+
+<pre class=example id=example-textencode-writable>

`<pre` also needs two spaces in front of it.

> @@ -1267,6 +1434,23 @@ requires buffering of scalar values.
  <dt><code><var>encoder</var> . <a attribute for=TextEncoder>encoding</a></code>
  <dd><p>Returns "<code>utf-8</code>".
 
+ <dt><code><var>encoder</var> . <a attribute for=TextEncoder>readable</a></code>
+ <dd>
+  <p>Returns a <a>readable stream</a> whose <a>chunks</a> are {{Uint8Array}}s resulting from running
+  <a>UTF-8</a>'s <a for=/>encoder</a> on the chunks written to {{TextEncoder/writable}}.
+
+ <dt><code><var>encoder</var> . <a attribute for=TextEncoder>writable</a></code>
+ <dd>
+  <p>Returns a <a>writable stream</a> which accepts string chunks and runs them through
+  <a>UTF-8</a>'s <a for=/>encoder</a> before making them available to {{TextEncoder/readable}}.
+
+  <p>Typically this will be used via the {{ReadableStream/pipeThrough()}} method on a {{ReadableStream}} source.
+
+<pre class=example id=example-textencode-writable>

Also, this should probably use `<code>` too, just like the other examples.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/encoding/pull/127#pullrequestreview-90521008