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

annevk commented on this pull request.

Thanks, this looks great. I mostly have a ton of nits.

> +     [=queue a task=] to [=fire a progress event=] called {{loadstart}} at the [=context object=].
+  1. Set |isFirstChunk| to false.
+
+  1. If |chunk| is fulfilled with an object whose `done` property is false and whose `value`
+     property is a `Uint8Array` object, run these steps:
+    1. Let |bs| be the [=byte sequence=] represented by the `Uint8Array` object.
+    1. Append |bs| to |bytes|.
+    1. If roughly 50ms have passed since these steps were last invoked,
+       [=queue a task=] to [=fire a progress event=] called {{progress}} at the [=context object=].
+    1. Set |chunk| to the result of [=read a chunk|reading a chunk=] from |stream| with |reader|.
+
+  1. Otherwise, if |chunk| is fulfilled with an object whose `done` property is true,
+     [=queue a task=] to run the following steps and abort this algorithm:
+    1. Set the [=context object=]'s {{FileReader/readyState}} to {{DONE}}.
+    1. Let |result| be the result of
+       [=package data=] given |bytes|, |type|, |blob|'s {{Blob/type}} and |encodingName|.

Add Oxford comma?

> +1. If {{FileReader/readyState}} = {{FileReader/LOADING}}
+   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|,

Oxford comma.

> +
+## 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:
+
+: DataURL
+:: Return |bytes| as a DataURL [[!RFC2397]] subject to the considerations below:
+
+  * Use |mimeType| as part of the Data URL if it is available
+    in keeping with the Data URL specification [[!RFC2397]].
+  * If |mimeType| is not available return a Data URL without a media-type. [[!RFC2397]].
+    Data URLs that do not have media-types [[RFC2046]]
+    must be treated as plain text by conforming user agents. [[!RFC2397]].

Let's remove this requirement (or in a follow-up) as it might contradict Fetch which defines the normative processing requirements for data URLs these days.

> +switches on |type|, and runs the associated steps:
+
+: DataURL
+:: Return |bytes| as a DataURL [[!RFC2397]] subject to the considerations below:
+
+  * Use |mimeType| as part of the Data URL if it is available
+    in keeping with the Data URL specification [[!RFC2397]].
+  * If |mimeType| is not available return a Data URL without a media-type. [[!RFC2397]].
+    Data URLs that do not have media-types [[RFC2046]]
+    must be treated as plain text by conforming user agents. [[!RFC2397]].
+
+: Text
+:: 1. Let |encoding| be null.
+   1. If the |encodingName| is present, set |encoding| to the result of
+      [=getting an encoding=] from |encodingName|.
+   1. If the [=getting an encoding=] step above returns failure, set |encoding| to null.

It seems you only need to check if encoding is failure at this point. That's cleaner than referencing an algorithm in a prior step.

> +:: Return |bytes| as a DataURL [[!RFC2397]] subject to the considerations below:
+
+  * Use |mimeType| as part of the Data URL if it is available
+    in keeping with the Data URL specification [[!RFC2397]].
+  * If |mimeType| is not available return a Data URL without a media-type. [[!RFC2397]].
+    Data URLs that do not have media-types [[RFC2046]]
+    must be treated as plain text by conforming user agents. [[!RFC2397]].
+
+: Text
+:: 1. Let |encoding| be null.
+   1. If the |encodingName| is present, set |encoding| to the result of
+      [=getting an encoding=] from |encodingName|.
+   1. If the [=getting an encoding=] step above returns failure, set |encoding| to null.
+   1. If |encoding| is null, and |mimeType| is present:
+     1. Let |type| be the result of [=parse a MIME type=] given |mimeType|.
+     1. If |type| is failure, abort these substeps.

Might be good to file a follow-up bug on removing this kind of anti-pattern. A preferred style would be checking that type is not failure and then nesting the further set of steps, or something equivalent.

>  
-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|.
+   If that threw an exception, abort these steps and rethrow that exception.

This is implied these days so no need to state it.

>  
-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|.
+   If that threw an exception, abort these steps and rethrow that exception.
+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.
+1. If |promise| fulfilled with a [=byte sequence=] |bytes|:
+  1. Return the result of [=package data=] given |bytes|, *Text*, |blob|'s {{Blob/type}} and |encoding|.

Oxford comma

> -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|.
+   If that threw an exception, abort these steps and rethrow that exception.
+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.
+1. If |promise| fulfilled with a [=byte sequence=] |bytes|:
+  1. Return the result of [=package data=] given |bytes|, *Text*, |blob|'s {{Blob/type}} and |encoding|.
+1. Otherwise:

No need for otherwise here as it can only happen if the above didn't happen, right?

>  
-1. Initiate a <a>read operation</a> using the <code>blob</code> argument,
-  and with the <a>synchronous flag</a> *set*.
-  If the <a>read operation</a> 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>
-  as a Data URL [[!RFC2397]] subject to the considerations below:
-
-  * Use the <code>blob</code>'s {{Blob/type}} attribute as part of the Data URL if it is available
-    in keeping with the Data URL specification [[!RFC2397]].
-  * If the {{Blob/type}} attribute is not available on the <code>blob</code> return a Data URL without a media-type. [[!RFC2397]].
-    Data URLs that do not have media-types [[RFC2046]] must be treated as plain text by conforming user agents. [[!RFC2397]].
+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|.
+   If that threw an exception, abort these steps and rethrow that exception.

This can be removed per above.

>  
-1. Initiate a <a>read operation</a> using the <code>blob</code> argument,
-  and with the <a>synchronous flag</a> *set*.
-  If the <a>read operation</a> 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> as an {{ArrayBuffer}}.
+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|.
+   If that threw an exception, abort these steps and rethrow that exception.

Same.

>  
-1. Initiate a <a>read operation</a> using the <code>blob</code> argument,
-  and with the <a>synchronous flag</a> *set*.
-  If the <a>read operation</a> 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> as an {{ArrayBuffer}}.
+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|.
+   If that threw an exception, abort these steps and rethrow that exception.
+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.
+1. If |promise| fulfilled with a [=byte sequence=] |bytes|:
+  1. Return the result of [=package data=] given |bytes|, *ArrayBuffer* and |blob|'s {{Blob/type}}.

Oxford comma.

> +<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|.
+1. Let |isFirstChunk| be true.
+1. [=In parallel=], while true:
+  1. Wait for |chunk| to be fulfilled or rejected.
+  1. If |chunk| is fulfilled, and |isFirstChunk| is true,
+     [=queue a task=] to [=fire a progress event=] called {{loadstart}} at the [=context object=].

So I'd argue Firefox is correct in firing this synchronously. That's also what XHR does iirc. Is there a problem with doing that?

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