See bug 649133 comment #17.

I doubt the usefulness of the extension. This is no more than a syntax sugar of JSON.parse(xhr.responseText) unless streaming JSON parser is used.

Well, there's also the fact that we'd use less memory since we can toss out the source text as soon as onload fires and we've parsed into a JSON object. Additionally, it would let us add streaming parsing in the future if it turns out that this is something that people use a lot for large JSON objects.

Masatoshi, Jonas, have you brought this up in WebApps WG? I think json makes sense and should be standardized.

Since the sending type is still plain text, the contentType is defaulted to "text/plain" and we need to set the it manually to "application/json" - let me know if we should change the behavior here.

Also please note this patch needs to go after Bug 678432 is checked in.

Comment on attachment 553984 [details] [diff] [review] Patch 1. Review of attachment 553984 [details] [diff] [review]: ----------------------------------------------------------------- Two higher-level comments: You should only do the JSON conversion once and remember the result (if successful). The same way that you fixed for arraybuffer returns. Sorry, I should have mentioned this earlier, my bad :( Second, don't do the testing using test_XHRSendData, that test is better adapted to testing various ways of sending data, whereas what you're testing here is a new way of receiving data. Instead modify test_XHR.html. And make sure to test that we return the same object if accessed multiple times. And that we throw if the response isn't valid JSON, and that we throw twice if you access the property twice. ::: content/base/src/nsXMLHttpRequest.cpp @@ +1000,5 @@ > > + case XML_HTTP_RESPONSE_TYPE_JSON: > + if (mState & XML_HTTP_REQUEST_DONE) { > + nsString bodyString; > + CopyUTF8toUTF16(mResponseBody.BeginReading(), bodyString); You can't assume that the response is UTF8. Instead convert the same way as we do for text. @@ +1004,5 @@ > + CopyUTF8toUTF16(mResponseBody.BeginReading(), bodyString); > + if (!JS_ParseJSON(aCx, > + (jschar*)PromiseFlatString(bodyString).get(), > + bodyString.Length(), aResult)) { > + NS_ERROR("Wrong JSON"); NS_ERROR is equivalent to an assertion, and we don't want to assert on things that are simply errors on the webpage (i.e. a webpage should never be able to cause us to assert). If this returns false, simple return NS_ERROR_FAILURE.

Fix!

Comment on attachment 554572 [details] [diff] [review] Patch 2. Review of attachment 554572 [details] [diff] [review]: ----------------------------------------------------------------- Still need to review the test. But looks good otherwise. ::: content/base/src/nsXMLHttpRequest.cpp @@ +1027,5 @@ > + case XML_HTTP_RESPONSE_TYPE_JSON: > + if (mState & XML_HTTP_REQUEST_DONE) { > + if (mResultJSON == JSVAL_VOID) { > + rv = CreateResponseParsedJSON(aCx); > + NS_ENSURE_SUCCESS(rv, rv); After parsing you can clear mResponseBuffer and mResponseBufferUnicode since they won't be used any more. However only do this in the case of parsing succeeding so that we throw the correct error if someone gets the property again.

Comment on attachment 554572 [details] [diff] [review] Patch 2. Review of attachment 554572 [details] [diff] [review]: ----------------------------------------------------------------- Looks good otherwise! ::: content/base/src/nsXMLHttpRequest.cpp @@ +1027,5 @@ > + case XML_HTTP_RESPONSE_TYPE_JSON: > + if (mState & XML_HTTP_REQUEST_DONE) { > + if (mResultJSON == JSVAL_VOID) { > + rv = CreateResponseParsedJSON(aCx); > + NS_ENSURE_SUCCESS(rv, rv); After parsing you can clear mResponseBuffer and mResponseBufferUnicode since they won't be used any more. However only do this in the case of parsing succeeding so that we throw the correct error if someone gets the property again. ::: content/base/test/responseIdentical.sjs @@ +11,5 @@ > + var bytes = [], avail = 0; > + while ((avail = bodyStream.available()) > 0) > + body += String.fromCharCode.apply(String, bodyStream.readByteArray(avail)); > + > + response.setHeader("Content-Type", "text/html", false); Make this "application/octet-stream" instead. ::: content/base/test/test_XHR.html @@ +18,5 @@ > +// test receiving as JSON > +function checkSetResponseTypeJSONThrows(xhr) { > + var didthrow = false; > + try { xhr.responseType = 'moz-json'; } catch (e) { didthrow = true; } > + ok(didthrow, "should have thrown when accessing responseType as JSON"); "should have thrown when setting responseType to moz-json" Also, no need to do this as a separate function @@ +31,5 @@ > + xhr.responseType = 'moz-json'; > + xhr.send(aJsonStr); > + > + var isResponseReceived = false; > + xhr.onreadystatechange = function() { Both the onreadystatechange function and the onload function can be replaced with the following code: if (!invalid) { is(JSON.stringify(xhr.response), aJsonStr); is(xhr.response, xhr.response, "returning the same object on each access"); } else { var didThrow = false; try { xhr.response } catch(ex) { didThrow = true; } ok(didThrow, "accessing response should throw"); didThrow = false; try { xhr.response } catch(ex) { didThrow = true; } ok(didThrow, "accessing response should throw"); } Also, you need to change the test to do the load synchronously, as to ensure that the load happens before the test finishes.

Comment on attachment 559279 [details] [diff] [review] Patch 3 - corrections according to Jonas' comments Review of attachment 559279 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed! ::: content/base/test/test_XHR.html @@ +29,5 @@ > + xhr.open("POST", 'responseIdentical.sjs', false); > + xhr.responseType = 'moz-json'; > + xhr.send(aJsonStr); > + > + var isResponseReceived = false; This variable isn't used anywhere.

Checked in to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/27c140d86b1b Thanks for the patch Shawn!! Very excited to have this support!

Comment on attachment 559301 [details] [diff] [review] Patch 4. Review+ by sicking >+ nsString bodyString; >+ ConvertBodyToText(bodyString); >+ if (!JS_ParseJSON(aCx, >+ (jschar*)PromiseFlatString(bodyString).get(), [bodyString is an nsString so it's already flat]

Documented: https://developer.mozilla.org/en/DOM/XMLHttpRequest#Properties (see the response and responseType properties) Also mentioned on Firefox 9 for developers.

For anyone that Google to this bug: the "moz-json" keyword have been replaced by "json" in bug 655727.

oops, bug 707142