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

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
> <sergiu.dumitriu@gmail.com> 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/

Received on Monday, 13 September 2010 18:10:59 UTC