389739 - (flock9108) XMLHttpRequest readyState does not define constants

Status: RESOLVED DUPLICATE of bug 649133

(Core :: DOM: Core & HTML, defect)

Description

[Matthew (lilmatt) Willis]

Attachment: adds constants for readyState results

The readyState integers for XMLHttpRequest are not defined as constants but are instead listed in a comment in the IDL.

This leads to code like this:

  this.req = CC[XMLHTTPREQUEST_CONTRACTID].
             createInstance(CI.nsIXMLHttpRequest);
  this.req.QueryInterface(CI.nsIJSXMLHttpRequest);
  this.req.open("GET", aUrl, true);
  var req = this.req;
  this.req.onreadystatechange = function (aEvt) {
    if (req.readyState == 4) {
      try {

We should move the values from the comments into constants for the readyStates.  This makes the code more readable and gets rid of some "magic numbers".

Comment 1

[Boris Zbarsky [:bzbarsky]]

How does this align with the W3C spec for this stuff?

Comment 2

[Boris Zbarsky [:bzbarsky]]

In particular, I'd rather not introduce constants that would not interoperate with other UAs...

Comment 3

[Matthew (lilmatt) Willis]

Good call.  They're named differently than the original comment in the idl.  I'll make a new patch using the W3C terms.

See http://www.w3.org/TR/XMLHttpRequest/#xmlhttprequest

Comment 4

[Matthew (lilmatt) Willis]

Attachment: rev1 - Use W3C terms for constants

Comment 5

[Matthew (lilmatt) Willis]

Attachment: rev2 - Removes stupidity

Shortens constant names since "XMLHTTPREQUEST" is included in CI.nsIXMLHttpRequest.

Also makes them unsigned shorts

Comment 6

[Matthew (lilmatt) Willis]

Attachment: rev3 - Use exact terms from spec per bz

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274082 [details] [diff] [review]
rev3 - Use exact terms from spec per bz

Thanks!

Comment 8

[Matthew (lilmatt) Willis]

Attachment: rev4 - Includes missing implementation

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274109 [details] [diff] [review]
rev4 - Includes missing implementation

Isn't "short" PRUint16?  Check the generated header from this IDL?

Comment 10

[Matthew (lilmatt) Willis]

Comment on attachment 274109 [details] [diff] [review]
rev4 - Includes missing implementation

Removing review request.  I will try again after I get sleep.

Comment 11

[Anne (:annevk)]

Please note that SENT has been renamed to HEADERS_RECEIVED in the editor's draft: http://dev.w3.org/cvsweb/~checkout~/2006/webapi/XMLHttpRequest/Overview.html?content-type=text/html;%20charset=utf-8

Comment 12

[Anne (:annevk)]

fantasai told me that OPEN clashed with open() because IDLs are not case-sensitive. I'm not really sure what a better name would be though. READY, maybe?

Comment 13

[Matthew (lilmatt) Willis]

(In reply to comment #12)
Microsoft previously used "LOADING"
http://msdn2.microsoft.com/en-us/library/ms753800.aspx

however they appear to now follow the draft:
http://msdn2.microsoft.com/en-us/library/ms534361.aspx

Perhaps we could simply prepend "READYSTATE_" to the constants?  IMO doing so makes them more readable in code as it explicity indicates these are readyState values.
  (ex: "XMLHttpRequest::READYSTATE_OPEN" vs."XMLHttpRequest::OPEN")

Comment 14

[Anne (:annevk)]

I wouldn't trust Microsoft documentation. It is inaccurate. (Microsoft employees admitted as much which is part of the reason that SENT hsa been renamed to HEADERS_RECEIVED for instance.)

Prepending it with READYSTATE_ is not consistent with similar APIs (such as HTMLMediaElement from HTML 5 or various DOM features) so I would rather not do that. The names should be as concise and accurate as possible, imo.

Comment 15

[Jeff Walden [:Waldo]]

Given that all the names except LOADING are past-tense, why not OPENED?

Comment 16

[Anne (:annevk)]

Cool, UNSENT, OPENED, HEADERS_RECEIVED, LOADING and DONE it is.

Comment 17

[Anne (:annevk)]

FWIW, the editor's draft mentioned in comment 11 has been updated to reflect this change.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Was fixed in bug 649133.