[XHR2] Comments on the 7 September working draft

Section 1 (Introduction), simple example:
- "test" is a very bad name for a function, how about "handleData"?
- this.readyState == 4 IMO would sound better as == XMLHttpRequest.DONE
- this.responseXML.getElementById('test').firstChild.data can throw a 
NPE in so many places, and it's just DOM manipulation irrelevant to 
XHR2; is that really needed? How about using just 
this.responseXML.getElementById('test')?
- It's a good practice to always use {} around if-else blocks, even if 
they're one-liners.
- The cyclomatic complexity of the handler method can be reduced if all 
the else blocks would be removed, as in:

> function handler() {
>  if(this.readyState == 4) {
>   // so far so good
>   if(this.status == 200 && this.responseXML != null && this.responseXML.getElementById('test').firstChild.data) {
>    // success!
>    test(this.responseXML.getElementById('test').firstChild.data);
>    return;
>   }
>   // missing data, fetched the wrong page, or network error...
>   test(null);
>  }
> }

- I'd apply other name improvements, resulting in:

> function handleData(data) {
>  // taking care of data
> }
>
> function responseHandler() {
>  if (this.readyState == XMLHttpRequest.DONE) {
>   // response fully received
>   if (this.status == 200 && this.responseXML != null && this.responseXML.getElementById('data')) {
>    // success!
>    handleData(this.responseXML.getElementById('data'));
>    return;
>   }
>   // missing data, fetched the wrong page, network error...
>   handleData(null);
>  }
> }
>
> var client = new XMLHttpRequest();
> client.onreadystatechange = responseHandler;
> client.open("GET", "unicorn.xml");
> client.send();

- In the third example, fetchStatus, (this.readyState == 4) should also 
be replaced with XMLHttpRequest.DONE.


Section 2.1, Dependencies:
- Shouldn't FileAPI be added, since XHR2 uses Blob and mentions File?


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?


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.
- "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."


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?


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.


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?
- "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?
- "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.
- "The entity body used in the request" -> "possibly null"
- "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?
- "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.
- 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.
- When is the upload object created? Should it be mentioned in the 
constructors section?
- Shouldn't Document, origin, base URL be mentioned here, or is it 
considered that 3.1 is enough?

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?
- 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"?
- 3. "subtract 0x20 from each byte in the range 0x61 (ASCII a) to 0x7A 
(ASCII z)" -> What a fancy way of saying "transform to uppercase"...
- 9, "If the "user:password" format..." -> What about just the "user"?
- 18 -> set the error flag to false?
- 19, typo: "Switch the --the-- state to OPENED"


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.
- "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?
- The code sample at the end needs some kind of introduction.


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.


Section 3.6.6, The withCredentials attribute
- Step 3, fail even if the assigned value is false?


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.
- 6, "Set the error flag to false." -> Why not move this to the open() 
method, where all the other flags are initialized?
- 9, same origin, asynchronous: "Make progress notifications" and "Make 
upload progress notifications" should be swapped, since upload occurs 
before download.
- 9, cross origin: "force preflight flag, The upload events flag" -> 
Why? What do the event listeners have to do with preflight requests?
- 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?
- "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?
- "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?
- "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)
- "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 */*.


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).
- 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.
- 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?)


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).


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"


Section 3.8, Events summary:
- I'd put timeout before load.
- For progress, I'd say "While sending and loading data" (sending first)


Section 4, FormData:
- Shouldn't a read method be included?
- Should it be made more clear that adding the same key with different 
values keeps the old values as well?

-- 
Sergiu Dumitriu
http://purl.org/net/sergiu/

-- 
Sergiu Dumitriu
http://purl.org/net/sergiu/

Received on Thursday, 9 September 2010 16:35:21 UTC