Re: [w3c/FileAPI] Refactor Blob URL Store definition. (#92)

domenic commented on this pull request.

Very excited for this! Hope my review was helpful :)

> @@ -1598,286 +1600,161 @@ A URL for Blob and File reference</h2>
 
 This section defines a scheme for a URL used to refer to {{Blob}} objects (and {{File}} objects).

Maybe do `[=URL/scheme=]` and `[=URL=]` here.

> +*This section is informative.*
+
+[=Blob URL|Blob (or object) URLs=] are URLs like
+<code>blob:http://example.com/550e8400-e29b-41d4-a716-446655440000</code>.
+This enables integration of {{Blob}}s and {{File}}s with other Web Platform APIs
+that are only designed to be used with URLs, such as the <{img}> element.
+[=Blob URLs=] can also be used to navigate to as well as to trigger downloads of
+locally generated data.
+
+For this purpose two static methods are exposed on the {{URL}} interface,
+{{URL/createObjectURL(blob)}} and {{URL/revokeObjectURL(url)}}.
+The first method creates a mapping from a URL to a Blob,
+and the second method revokes said mapping.
+As long as a mapping exist a Blob  can't be garbage collected,
+so some care must be taken to revoke a URL as soon as a reference is no longer needed.
+All URLs are revoked when the global that created the URL itself goes away.

In this paragraph, `Blob` should be `{{Blob}}`, I think.

> +Introduction</h3>
+
+*This section is informative.*
+
+[=Blob URL|Blob (or object) URLs=] are URLs like
+<code>blob:http://example.com/550e8400-e29b-41d4-a716-446655440000</code>.
+This enables integration of {{Blob}}s and {{File}}s with other Web Platform APIs
+that are only designed to be used with URLs, such as the <{img}> element.
+[=Blob URLs=] can also be used to navigate to as well as to trigger downloads of
+locally generated data.
+
+For this purpose two static methods are exposed on the {{URL}} interface,
+{{URL/createObjectURL(blob)}} and {{URL/revokeObjectURL(url)}}.
+The first method creates a mapping from a URL to a Blob,
+and the second method revokes said mapping.
+As long as a mapping exist a Blob  can't be garbage collected,

"As long as the mapping exists the Blob can't...", "...revoke the URL...", "as soon as the reference..."

> +that are only designed to be used with URLs, such as the <{img}> element.
+[=Blob URLs=] can also be used to navigate to as well as to trigger downloads of
+locally generated data.
+
+For this purpose two static methods are exposed on the {{URL}} interface,
+{{URL/createObjectURL(blob)}} and {{URL/revokeObjectURL(url)}}.
+The first method creates a mapping from a URL to a Blob,
+and the second method revokes said mapping.
+As long as a mapping exist a Blob  can't be garbage collected,
+so some care must be taken to revoke a URL as soon as a reference is no longer needed.
+All URLs are revoked when the global that created the URL itself goes away.
+
+<h3 id="url-model">
+Model</h3>
+
+Each user agent must maintain a <dfn id="BlobURLStore" export>Blob URL Store</dfn>.

Nit: I think generally we don't capitalize terms defined this way, so e.g. "blob URL store" and "blob URL entry" would be more conventional.

> +
+Each user agent must maintain a <dfn id="BlobURLStore" export>Blob URL Store</dfn>.
+A [=Blob URL Store=] is a [=map=]
+where [=map/keys=] are [=valid URL strings=]
+and [=map/values=] are [=Blob URL Entries=].
+
+A <dfn>Blob URL Entry</dfn> consists of
+an <dfn for="Blob URL Entry">object</dfn> (typically a {{Blob}},
+but other specs can extend this to refer to other types of objects),
+and an <dfn for="Blob URL Entry">environment</dfn> (an [=environment settings object=]).
+
+[=map/Keys=] in the [=Blob URL Store=] (also known as <dfn lt="Blob URL" export>Blob URLs</dfn>)
+are [=valid URL strings=] that when [=URL parser|parsed=]
+result in a [=/URL=] with a [=url/scheme=] equal to <code>"blob"</code>,
+a [=url/host=] equal to the [=environment settings object/origin=]
+of the [=environment settings object=] in which the URL was create,

s/was create/was created

Is this the origin of the corresponding blob URL entry's environment? If so maybe I would make this correspondence explicit somewhere.

If it does *not* correspond, then an example of when that's the case would also be useful.

>  
-<h4 id="originOfBlobURL">
-Origin of Blob URLs</h4>
+1. Let |store| be the user agent's [=Blob URL Store=];

Semicolon should be period

>  
-[=CORS protocol|Cross-origin requests=] on Blob URLs must return a <a>network error</a>.
+1. Let |store| be the user agent's [=Blob URL Store=];
+1. Let |url string| be the result of [=URL serializer|serializing=] |url|.
+1. Remove |store|[|url string|].

Since you seem to like linking to Infra things, you can link to remove too :)

>  
 1. Let |result| be the empty string.
-  Append the string "blob"
-  (that is, the Unicode code point sequence U+0062, U+006C, U+006F, U+0062)
+1. Append the string "blob"
+  (that is, the Unicode [=code point=] sequence U+0062, U+006C, U+006F, U+0062)

Strings should be "`blob`", i.e. `<code>` inside the quotes. Here and for "null" below.

>  
-<a>Blob URLs</a> are <dfn lt="dereference">dereferenced</dfn>
-when the user agent retrieves the resource identified by the Blob URL
-and returns it to the requesting entity.
-This section provides guidance on [=/requests=] and [=/responses=]
+1. [=Assert=]: |url|'s [=url/scheme=] is <code>"blob"</code>.
+1. Let |store| be the user agent's [=Blob URL Store=];

semicolon should be period

>  
-<a>Blob URLs</a> are <dfn lt="dereference">dereferenced</dfn>
-when the user agent retrieves the resource identified by the Blob URL
-and returns it to the requesting entity.
-This section provides guidance on [=/requests=] and [=/responses=]
+1. [=Assert=]: |url|'s [=url/scheme=] is <code>"blob"</code>.
+1. Let |store| be the user agent's [=Blob URL Store=];
+1. Let |url string| be the result of [=URL serializer|serializing=] |url| with the *exclude fragment flag* set.
+1. Let |entry| be the result of [=map/getting=] |store|[|url string|].

You can just say "Let |entry| be |store|[|url string|]." Or even "Return |store|[|url string|]."

>  
-This section provides sample exchanges between web applications and user agents using <a>Blob URLs</a>.
-A [=/request=] can be triggered using HTML markup of the sort
-<code class="lang-markup">&lt;img src="blob:http://example.org:8080/550e8400-e29b-41d4-a716-446655440000"></code>.
-These examples merely illustrate the [=/request=] and [=/response=];
-web developers are not likely to interact with all the headers,
-but the {{XMLHttpRequest/getAllResponseHeaders()}} method of {{XMLHttpRequest}}, if used,
-will show relevant [=/response=] headers.
+1. [=Assert=]: |url|'s [=url/scheme=] is <code>"blob"</code>.
+1. Let |entry| be the result of [=Blob URL/resolve|resolving=] |url|.
+1. If |entry| is not undefined, return |entry|'s [=Blob URL Entry/environment=]'s [=environment settings object/origin=].

Hmm, so unlike JS, we don't say that getting something from a map returns undefined when it doesn't exist. Instead you should probably do something like "If map[x] exists, return map[x]; otherwise, return failure." Then test for failure here. Or null instead of failure, perhaps.

> @@ -1898,58 +1775,54 @@ unless the user agent also implements the URL [[URL]] specification.
 In other words, <code>URL.prototype</code> must evaluate to true if the user agent implements the URL [[URL]] specification,

This "ECMAScript user agents" section seems unnecessary these days, where everyone has a `URL` interface.

> +
+</div>
+
+<div algorithm="revokeObjectURL">
+The <dfn method for=URL id="dfn-revokeObjectURL">revokeObjectURL(|url|)</dfn> static method must run these steps:
+
+1. [=Remove an entry from the Blob URL Store=] for |url|.
+1. Let |settings| be the [=current settings object=]
+1. Let |origin| be the origin of |url|. FIXME
+1. If |origin| is not [=same origin=] with |settings|'s [=environment settings object/origin=], return.
+1. [=Remove an entry from the Blob URL Store=] for |url|.
+
+Issue: Do we actually need a same-origin check? It doesn't seem like this is something browser currently do, but it seems nice to have.
+
+Note: This means that rather than throwing some kind of error, attempting to revoke a URL that isn't registered will silently fail.
+User agents may display a message on the error console is this happens.

s/may/might, or make this normative

> +
+<div algorithm="revokeObjectURL">
+The <dfn method for=URL id="dfn-revokeObjectURL">revokeObjectURL(|url|)</dfn> static method must run these steps:
+
+1. [=Remove an entry from the Blob URL Store=] for |url|.
+1. Let |settings| be the [=current settings object=]
+1. Let |origin| be the origin of |url|. FIXME
+1. If |origin| is not [=same origin=] with |settings|'s [=environment settings object/origin=], return.
+1. [=Remove an entry from the Blob URL Store=] for |url|.
+
+Issue: Do we actually need a same-origin check? It doesn't seem like this is something browser currently do, but it seems nice to have.
+
+Note: This means that rather than throwing some kind of error, attempting to revoke a URL that isn't registered will silently fail.
+User agents may display a message on the error console is this happens.
+
+Note: Attempts to dereference |url| after it has been revoked will result in a [=network error=].

s/dereference/fetch?

-- 
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/92#pullrequestreview-80965331