Re: [w3c/FileAPI] Rewrite FileReader definitions. (#118)

domenic approved this pull request.

Generally LGTM, with some suggestions which are mostly scope-creep.

> @@ -251,6 +253,25 @@ Their [=deserialization step=], given |serialized| and |value|, are:
 
 2. Set |value|'s underlying byte sequence to |serialized|.\[[ByteSequence]].
 
+<div algorithm="get stream">
+A {{Blob}} |blob| has an associated <dfn for=Blob>get stream</dfn> algorithm,
+which runs these steps:
+
+1. Let |stream| be the result of [=construct a ReadableStream object|constructing=] a
+   {{ReadableStream}} object.
+1. Run the following steps [=in parallel=]:
+  1. While not all bytes of |blob| have been read:
+    1. Let |bytes| be the byte sequence that results from reading a [=chunk=] from |blob|.
+    1. If a [=file read error=] occured while reading |bytes|, [$ReadableStream/error$]
+       |stream| with a [=failure reason=] and abort these steps.

Probably scope-creep to fix this here, but it's not very clear how to get from "failure reason" to `an "X" DOMException`.

> @@ -251,6 +253,25 @@ Their [=deserialization step=], given |serialized| and |value|, are:
 
 2. Set |value|'s underlying byte sequence to |serialized|.\[[ByteSequence]].
 
+<div algorithm="get stream">
+A {{Blob}} |blob| has an associated <dfn for=Blob>get stream</dfn> algorithm,
+which runs these steps:
+
+1. Let |stream| be the result of [=construct a ReadableStream object|constructing=] a
+   {{ReadableStream}} object.
+1. Run the following steps [=in parallel=]:
+  1. While not all bytes of |blob| have been read:
+    1. Let |bytes| be the byte sequence that results from reading a [=chunk=] from |blob|.

```suggestion
    1. Let |bytes| be the [=byte sequence=] that results from reading a [=chunk=] from |blob|.
```

> @@ -251,6 +253,25 @@ Their [=deserialization step=], given |serialized| and |value|, are:
 
 2. Set |value|'s underlying byte sequence to |serialized|.\[[ByteSequence]].
 
+<div algorithm="get stream">
+A {{Blob}} |blob| has an associated <dfn for=Blob>get stream</dfn> algorithm,
+which runs these steps:
+
+1. Let |stream| be the result of [=construct a ReadableStream object|constructing=] a
+   {{ReadableStream}} object.
+1. Run the following steps [=in parallel=]:
+  1. While not all bytes of |blob| have been read:
+    1. Let |bytes| be the byte sequence that results from reading a [=chunk=] from |blob|.
+    1. If a [=file read error=] occured while reading |bytes|, [$ReadableStream/error$]
+       |stream| with a [=failure reason=] and abort these steps.
+    1. [=ReadableStream/Enqueue=] |bytes| into |stream|.

Readable streams, at least currently, take JS objects, not Infra things like byte sequences. For example https://fetch.spec.whatwg.org/#concept-read-all-bytes-from-readablestream assumes that the chunks are Uint8Arrays. So, if the goal is to integrate with that, you'd want to explicitly construct a Uint8Array.

Alternately, you could leave this as byte sequences as that's arguably a more correct long-term infrastructure choice. It's just the way that readable streams are defined right now that's making things difficult. I'm not sure what's best.

>  };
 </pre>
 
+<div algorithm="read operation">
+A {{FileReader}} has an associated <dfn id=readOperation>read operation</dfn> algorithm,
+given |blob|, a |type| and an optional |encodingName|,

```suggestion
which given |blob|, a |type| and an optional |encodingName|,
```

>  };
 </pre>
 
+<div algorithm="read operation">
+A {{FileReader}} has an associated <dfn id=readOperation>read operation</dfn> algorithm,
+given |blob|, a |type| and an optional |encodingName|,
+runs the following steps:
+
+1. Set the [=context object=]'s {{FileReader/result}} to `null`.

Maybe not something to clean up now, but I'd rather say "A FileReader _fr_ has an associated..." and then use _fr_ instead of the context object throughout.

>  };
 </pre>
 
+<div algorithm="read operation">
+A {{FileReader}} has an associated <dfn id=readOperation>read operation</dfn> algorithm,
+given |blob|, a |type| and an optional |encodingName|,
+runs the following steps:
+
+1. Set the [=context object=]'s {{FileReader/result}} to `null`.
+1. Set the [=context object=]'s {{FileReader/error!!attribute}} to `null`.

Setting readonly attributes is not great; this should set an internal slot/associated value which those getters return the value of.

>  };
 </pre>
 
+<div algorithm="read operation">
+A {{FileReader}} has an associated <dfn id=readOperation>read operation</dfn> algorithm,
+given |blob|, a |type| and an optional |encodingName|,
+runs the following steps:
+
+1. Set the [=context object=]'s {{FileReader/result}} to `null`.
+1. Set the [=context object=]'s {{FileReader/error!!attribute}} to `null`.
+1. Let |stream| be the result of calling [=get stream=] on |blob|.
+1. Let |reader| be the result of [=get a reader|getting a reader=] from |stream|.
+1. Let |bytes| by an empty [=byte sequence=].
+1. Let |chunk| be the result of [=read a chunk|reading a chunk=] from |stream| with |reader|.

It's confusing to me to use _chunk_ to refer to a promise; I'd prefer _chunkPromise_.

> -    until one of the <a>read methods</a> have been called on it.
-  <dt><dfn id="dfn-loading">LOADING</dfn> (numeric value 1)
-  <dd>A {{File}} or {{Blob}} is being read.
-    One of the <a>read methods</a> is being processed,
-    and no error has occurred during the read.
-  <dt><dfn id="dfn-done">DONE</dfn> (numeric value 2)
-  <dd>The entire {{File}} or {{Blob}} has been read into memory,
-    OR a <a>file read error</a> occurred,
-    OR the read was aborted using {{FileReader/abort()}}.
-    The {{FileReader}} is no longer reading a {{File}} or {{Blob}}.
-    If {{FileReader/readyState}} is set to {{FileReader/DONE}}
-    it means at least one of the <a>read methods</a> have been called on this {{FileReader}}.
-</dl>
+The {{FileReader/readyState}} attribute tells you in which state the object is:
+
+: {{EMPTY}} (numeric value 0)

Personally I like having the dfns here and the IDL link to them. For constants it could go either way, but right now the readyState getter is not defined anywhere. (Related to the internal slots thing.)

> +   set {{FileReader/readyState}} to {{FileReader/DONE}}
+   and {{FileReader/result}} to `null`.
+1. If there are any [=tasks=] from the [=context object=]
+   on the [=file reading task source=] in an affiliated [=queue a task|task queue=],
+   then remove those [=tasks=] from that task queue.
+1. [=terminate an algorithm|Terminate the algorithm=] for the [=read method=] being processed.
+1. [=Fire a progress event=] called {{abort}} at the [=context object=].
+1. If [=context object=]'s {{FileReader/readyState}} is not {{LOADING}},
+   [=fire a progress event=] called {{loadend}} at the [=context object=].
+
+## Packaging data ## {#packaging-data}
+
+<div algorithm="package data">
+A {{Blob}} has an associated <dfn for=Blob>package data</dfn> algorithm,
+given |bytes|, a |type|, a optional |mimeType|, and a optional |encodingName|,
+switches on |type|, and runs the associated steps:

```suggestion
which switches on |type| and runs the associated steps:
```

>  
-1. Initiate a <a>read operation</a> using the <code>blob</code> argument,
-  and with the <a>synchronous flag</a> *set*.
-  If the read operation returns failure,
-  throw the appropriate exception as defined in [[#dfn-error-codes]].
-  <a>Terminate this algorithm</a>.
-1. If no error has occurred,
-  return the |result| of the <a>read operation</a> represented as a string
-  in a format determined through the <a>encoding determination</a> algorithm.
+1. Let |stream| be the result of calling [=get stream=] on |blob|.
+1. Let |reader| be the result of [=get a reader|getting a reader=] from |stream|.
+1. Let |promise| be the result of [=read all bytes|reading all bytes=] from |stream| with |reader|.
+1. Wait for |promise| to be fulfilled or rejected.

Not the most sound thing to do with real promises, but let's pretend these are spec promises, like they really should be if streams had a better infrastructure :).

-- 
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/FileAPI/pull/118#pullrequestreview-214749855