Re: [XHR2] Comments on the 7 September working draft

Wow, thanks for this!

On Thu, 09 Sep 2010 18:34:02 +0200, Sergiu Dumitriu  
<sergiu.dumitriu@gmail.com> wrote:
> Section 1 (Introduction), simple example:
> [...]

I made various improvements along the lines you suggested.


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

Used this.DONE instead.


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


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


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


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


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


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


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


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


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


> - "The entity body used in the request" -> "possibly null"

Fixed.


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


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


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


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


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


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


> - 9, "If the "user:password" format..." -> What about just the "user"?

That is mentioned shortly after.


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


> - 19, typo: "Switch the --the-- state to OPENED"

Fixed.


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


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


> - The code sample at the end needs some kind of introduction.

Isn't it self-explanatory? I.e. with the comments?


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


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

Yes, for consistency.


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


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


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


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


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


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


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


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


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


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


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


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


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


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


> Section 4, FormData:
> - Shouldn't a read method be included?

Why?


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

Cheers,


-- 
Anne van Kesteren
http://annevankesteren.nl/

Received on Monday, 13 September 2010 10:12:58 UTC