Last Comment Bug 655727 - Implement responseType='moz-json'
: Implement responseType='moz-json'
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Shawn Gong
:
Mentors:
Depends on: 649133
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-09 08:36 PDT by Masatoshi Kimura [:emk]
Modified: 2014-01-19 11:50 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1. (6.22 KB, patch)
2011-08-17 19:50 PDT, Shawn Gong
jonas: review-
Details | Diff | Splinter Review
Patch 2. (13.36 KB, patch)
2011-08-19 16:01 PDT, Shawn Gong
jonas: review-
Details | Diff | Splinter Review
Patch 3 - corrections according to Jonas' comments (12.84 KB, patch)
2011-09-08 14:04 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
Patch 4. Review+ by sicking (12.81 KB, patch)
2011-09-08 15:13 PDT, Shawn Gong
hippopotamus.gong: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2011-05-09 08:36:51 PDT
See bug 649133 comment #17.
Comment 1 Masatoshi Kimura [:emk] 2011-05-11 04:35:35 PDT
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.
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-11 11:16:55 PDT
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.
Comment 3 Olli Pettay [:smaug] 2011-07-07 14:34:15 PDT
Masatoshi, Jonas, have you brought this up in WebApps WG?
I think json makes sense and should be standardized.
Comment 4 Shawn Gong 2011-08-17 19:50:53 PDT
Created attachment 553984 [details] [diff] [review]
Patch 1.

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.
Comment 5 Shawn Gong 2011-08-17 19:52:20 PDT
Also please note this patch needs to go after Bug 678432 is checked in.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-18 02:27:00 PDT
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.
Comment 7 Shawn Gong 2011-08-19 16:01:49 PDT
Created attachment 554572 [details] [diff] [review]
Patch 2.

Fix!
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-19 19:37:26 PDT
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 9 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-07 16:56:49 PDT
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 10 Shawn Gong 2011-09-08 14:04:02 PDT
Created attachment 559279 [details] [diff] [review]
Patch 3 - corrections according to Jonas' comments
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-08 15:06:13 PDT
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.
Comment 12 Shawn Gong 2011-09-08 15:13:57 PDT
Created attachment 559301 [details] [diff] [review]
Patch 4. Review+ by sicking
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-08 18:16:56 PDT
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 15 neil@parkwaycc.co.uk 2011-09-12 04:11:54 PDT
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]
Comment 16 Eric Shepherd [:sheppy] 2011-12-02 13:20:54 PST
Documented:

https://developer.mozilla.org/en/DOM/XMLHttpRequest#Properties

(see the response and responseType properties)

Also mentioned on Firefox 9 for developers.
Comment 17 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-01-29 21:02:58 PST
For anyone that Google to this bug: the "moz-json" keyword have been replaced by "json" in bug 655727.
Comment 18 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-01-29 21:04:09 PST
oops, bug 707142

Note You need to log in before you can comment on or make changes to this bug.