On 09/13/2010 12:12 PM, Anne van Kesteren wrote:
Wow, thanks for this!
You're welcome.
On Thu, 09 Sep 2010 18:34:02 +0200, Sergiu Dumitriu
<[email protected]> wrote:
Section 1 (Introduction), simple example:
[...]
I made various improvements along the lines you suggested.
The refactoring is not quite equivalent:
if(this.readyState == this.DONE && this.status == 200) {
...
}
processData(null);
processData will be called even when this.readyState != DONE.
- In the third example, fetchStatus, (this.readyState == 4) should
also be replaced with XMLHttpRequest.DONE.
Used this.DONE instead.
OK.
Section 2.1, Dependencies:
- Shouldn't FileAPI be added, since XHR2 uses Blob and mentions File?
Fair enough, added. Not really sure how useful this section is compared
with simply using the terminology section...
Well, since the section is there, it should at least be somehow
complete. OK with the added text.
Section 2.2, Terminology:
- "To dispatch a readystatechange event means... and which uses the
Event interface" -> Is "uses" the proper word here? Implements? Or
isn't the fact that it is an event enough to suggest that it
implements the Event interface?
If it's an event saying uses Event is enough.
OK.
Section 3,1, Origin and Base URL:
- Where exactly is the Document assigned to the XHR object? It's not
mentioned later in the constructor (3.3), and in the open() method
it's assumed to be there already.
They are simply always linked.
OK.
- "In environments where the global object is represented by the
Window object the XMLHttpRequest object has an associated
XMLHttpRequest Document which is the Document object associated with
the Window object for which the XMLHttpRequest --interface-- object
was created." ->
"In environments where the global object is represented by the Window
object, _each_ XMLHttpRequest object has an associated XMLHttpRequest
Document which is the Document object associated with the Window
object for which _that_ XMLHttpRequest object was created."
The whole specification is singular which I think is fine. And interface
object is important here. It means the object you get back from
window.XMLHttpRequest
OK.
Section 3.4, Event Handler Attributes:
- "The following is the event handler attribute... that must be
supported as DOM attribute solely by the XMLHttpRequest object: " ->
What about AnonXHR?
It's just a subclass. Do we really need to state everywhere that
whatever applies to XMLHttpRequest applies to AnonXMLHttpRequest too?
No, it's OK as it is.
Section 3.5, States:
- "The DONE state has an associated error flag" -> This flag is used
starting with the OPENED state, so maybe the wording should be
changed, as in:
An *error flag* indicates some type of network error or abortion, and
is used for distinguishing between successful and failed requests in
the DONE state.
Reworded.
OK with the new text.
Section 3.6, Request:
- "The request method, The method used in the request" -> Could the
second method be replaced with "HTTP method" to reduce possible
ambiguities?
Done.
OK.
- "The request URL, The URL used in the request" -> Could it be made
more obvious that this is the resolved, absolute URL, and not the
relative one passed as an argument?
Why does that matter here?
Less ambiguity?
- "The author request headers" -> Does this list contain the default
user-agent headers? No, so maybe "user-added" should be mentioned
somewhere in there, since these aren't ALL the headers that will be used:
A list consisting of user-generated HTTP header name/value pairs to
be used in the request.
That is why the word "author" is in there. We typically use user to
refer to the end user of the product. I.e. the one operating the
browser. Authors are those that write the web pages, etc.
OK.
- "The entity body used in the request" -> "possibly null"
Fixed.
OK.
- "The amount of milliseconds a request can take before being
terminated. Initially zero" -> This might suggest that by default
requests timeout instantly. Should it be mentioned here that 0 means
no timeout?
Clarified.
OK.
- "The upload events flag, Used to determine whether to send upload
progress events for cross-origin requests. The flag is either true or
false" -> Is it only for cross-origin requests? The send() algorithm
doesn't say so. Perhaps this is more correct:
The *upload complete* flag, Used to determine when to stop sending
upload progress events. The flag is either true or false.
The *upload events* flag, Used to determine whether to send upload
progress events. The flag is either true or false.
The send() algorithm does say so. It is only used for cross-origin
requests.
OK. Still, maybe the proposed text for the upload complete flag could be
used?
- Looking for where the upload events flag is mentioned in the
specification, it's very badly named, described and used. It's only
read as the value for the force preflight flag of the cross-origin
request, it's activated only if there are upload event listeners in
the send() method, and is described as a condition for sending upload
progress events, although it's not mentioned at all in the upload
progress events algorithm. Maybe it could be removed completely.
No, we cannot remove it. It prevents revealing whether a cross-origin
server exists.
- When is the upload object created? Should it be mentioned in the
constructors section?
No, it's just always linked to the XMLHttpRequest object. Like
DOMImplementation is to Document.
OK.
- Shouldn't Document, origin, base URL be mentioned here, or is it
considered that 3.1 is enough?
I think it is better in 3.1 because they have provisions that allow e.g.
Web Workers to set them differently.
OK.
Section 3.6.1, The open() method:
- Why is it allowed to send user/password in the URL, but not as
arguments, for cross-origin requests?
Well, whether that is allowed is an open issue. I added a comment to
make sure that if we allow it is not allowed for cross-origin requests.
OK.
- 1.3, "...and let it be a globally unique identifier if the anonymous
flag is true" -> I don't get this, what kind of "identifier"? What
kind of "globally"? What kind of "unique"?
This is standard origin terminology. It's just an opaque identifier.
OK.
- 9, "If the "user:password" format..." -> What about just the "user"?
That is mentioned shortly after.
OK.
- 18 -> set the error flag to false?
That is already done during send() and cannot be moved because abort()
would function differently if we did that.
OK.
- 19, typo: "Switch the --the-- state to OPENED"
Fixed.
OK.
Section 3.6.2, The setRequestHeader() method:
- "Appends an header to the list of author request headers or if the
header is already in the author request headers its value appended
to." -> Weird phrasing. Suggestion:
Appends a header to the list of author request headers or, if
/header/ is already in the author request headers list, combine
/value/ and the existing value for this header.
Fixed.
OK.
- "Throws a SYNTAX_ERR exception if header is not a valid HTTP header
field name or if value is not a valid HTTP header field value." ->
Link for field name and field value?
This is a non-normative introduction. Explaining here in full detail how
DOMString does not or does map to a set of bytes would make it
unreadable I think.
OK.
- The code sample at the end needs some kind of introduction.
Isn't it self-explanatory? I.e. with the comments?
Yes it is; I was talking about something like the text in the examples
from the first section:
"Some simple code showcasing what happens when setting the same header
twice:"
Section 3.6.3, The timeout attribute:
- Shouldn't negative values be forbidden, or treated specially (set to
0 if negative)? Although the IDL signature of the field states that it
should be an unsigned long, in ECMAScript, which is the main target of
the specification, there are only Numbers.
Yes, but Web IDL handles that.
OK.
Section 3.6.6, The withCredentials attribute
- Step 3, fail even if the assigned value is false?
Yes, for consistency.
OK.
Section 3.6.8, The send() method:
- 5, "If the asynchronous flag is true and one or more event listeners
are registered on the XMLHttpRequestUpload object" -> What happens if
event listeners are added afterwards? Create, open, send, add
listeners -> no events.
Indeed, otherwise more information is revealed than intended.
- 6, "Set the error flag to false." -> Why not move this to the open()
method, where all the other flags are initialized?
See above, would differ for abort().
How exactly? abort() doesn't read the error flag.
- 9, same origin, asynchronous: "Make progress notifications" and
"Make upload progress notifications" should be swapped, since upload
occurs before download.
Done (though it should not matter in this case).
OK.
- 9, cross origin: "force preflight flag, The upload events flag" ->
Why? What do the event listeners have to do with preflight requests?
If you have event listeners you need a preflight otherwise the existence
of a server can be more easily determined.
OK, I think I get it now. When registering listeners, even if the
request will fail since it's not allowed by the remote server, the
events fired might still give out some information about the server. Am
I right?
- 9, cross origin, asynchronous -> Aren't [upload] progress events
fired for cross-origin requests? Or is their mention in the following
section enough, to make it clear that no events are sent in the
preflight request?
It should be.
OK.
- "If authentication fails, Authorization is not in the list of author
request headers, request username is null, and request password is
null, user agents should prompt the end user for their username and
password." + "They are also not prompted for cross-origin requests."
=> should a same-origin condition be included in the first text?
Fixed, simplified the text a lot too.
OK with the new text.
I just realized that AnonXHR is not mentioned here. I guess anonymous
requests should never prompt for username/password, right?
- "If the user agent supports HTTP State Management it should persist,
discard and send cookies (as received in the Set-Cookie response
header, and sent in the Cookie header) as applicable." -> Shouldn't
the anonymous flag be mentioned here?
Maybe this paragraph should just be dropped? HTML5 does not have an
equivalent sentence. FWIW, CORS is very clear on the behavior here.
Personally, I'd keep it there. It was present in XHR1, and CORS deals
only with cross-origin requests.
- "If not set through setRequestHeader() Accept-Language should be set
as appropriate" -> Maybe something like this could be added:
(e.g. the preferred language or languages configured in the user agent)
I prefer leaving advice like this to HTTP.
OK.
- "If not set through setRequestHeader() Accept must be set with as
value */*." -> Is this what current browsers do? I though they set
some combination of xml and html, maybe javascript/json, and then */*.
Some do, some don't. Hopefully they will all converge. The test suite is
testing this.
OK.
Section 3.6.9, Infrastructure for the send() method:
- When there are multiple conditions for a step, maybe an ", or"
should be added between items to avoid confusion (I admin I spent a
few seconds trying to understand when are bytes received, yet there's
no response body).
I looked at doing this and it seems it would make it more confusing. As
there are already so many boolean conditions. Is the icon not clear
enough? From a markup point of view this should all "work". :-)
OK, very slightly confusing (maybe it's just me), but indeed the markup,
icons and text should be enough.
- Request error algorithm, step 11, "If the upload complete flag is
false" -> This could be moved between steps 8 and 9, since upload is
logically before download, so the upload listener should be notified
first of the error.
Fair enough, moved.
OK.
- Request error algorithm, step 12, "Terminate the overall algorithm"
-> Which algorithm? If it's the request error one, then it's already
finishing without this explicit step. If it's another one, it should
be mentioned more clearly (terminate the send algorithm?)
Good point, removed this.
OK.
Section 3.7.3, The getResponseHeader() method:
- The code sample at the end is the only one indented with two spaces.
For consistency, either change it so that it uses one-space
indentation, or change the other samples to use two spaces (personally
I prefer two spaces since the result is more readable).
Two spaces everywhere it is.
Almost, there's still the comment inside the processData function in
Section 1.
Section 3.7.5, Response entity body:
- Document response entity body, step 3: "...and then terminate this
algorithm" -> Shouldn't the document be returned?
- Document response entity body, step 4: "terminate these steps return
null" -> missing _and_? "return null and terminate these steps"
Fixed.
OK.
Section 3.8, Events summary:
- I'd put timeout before load.
- For progress, I'd say "While sending and loading data" (sending first)
Did not move timeout, did reword progress.
OK. My initial thinking was that timeout is an error, and should be
close to the other errors, while load is the desired success which
occurs when no other error intervenes. Now I read it as a chronological
if..elseif sequence, and indeed it makes more sense when read as:
- elseif the data arrives, send a load event;
- elseif too much time passed and the response is not yet complete, send
a timeout event;
Section 4, FormData:
- Shouldn't a read method be included?
Why?
In case different code components fill in parts of the request data and
they want to check that they're not overriding somebody else's data, in
case that some field names should have only one value, or in case one
component wants to change/remove the value placed by an earlier
component (which calls for a remove method as well, leading to a
full-fledged map interface :( ). Maybe it gets too complex, so it should
be left as it is now, complex scenarios should be handled with a custom
object which is then converted to a FormData
- Should it be made more clear that adding the same key with different
values keeps the old values as well?
Isn't that what append means?
It sure does, never mind.
--
Sergiu Dumitriu
http://purl.org/net/sergiu/
