Bug 49633 – Add .responseType and .response to XMLHttpRequest

• Component: XML
• Status: RESOLVED FIXED

[Chris Rogers]

Add .responseType and .response to XMLHttpRequest

[Chris Rogers]

Created attachment 74065 [details]
Patch

[WebKit Review Bot]

Attachment 74065 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6202001

[Chris Rogers]

This is an initial patch of an implementation for the .responseType and .response attributes which are being discussed in the public webapps mailing list in this thread:
   Re: XHR responseArrayBuffer attribute: suggestion to replace "asBlob" with "responseType"

The idea is, after a call to open(), but before send() is called, optionally set xhr.responseType to one of the following:

"" // empty string is default behavior (text)
"text"
"document"
"blob"
"arraybuffer"


* The patch has been tested very basically to verify it's able to get binary data in an ArrayBuffer if .responseType is set to "arraybuffer".  Otherwise, the current XMLHttpRequest layout tests all pass.

* New layout tests still need to be added

* the return type on the .response attribute should by "Any" in the .idl, but I don't know how to do that and declare it as returning DOMString, but this doesn't matter since the custom getter ignores that and returns whatever type of object is appropriate.

* the custom bindings for V8 are not yet in the patch, but will closely match the JSC version.

* should we throw an exception or return an empty string if .responseText is accessed, but the .responseType does not match?  Currently I'm throwing an exception.

[Alexey Proskuryakov]

Looks reasonable to me, although passing a string wouldn't be my preference.

JSXMLHttpRequest::markChildren() needs to mark the response. And looking at it, I'm surprised that it doesn't mark responseXML - that's likely a bug.

+    } else {
+        ec = SYNTAX_ERR;
+    }

There should be no braces around single line blocks.

+    enum ResponseTypeCode {
+        RESPONSETYPE_DEFAULT,
+        RESPONSETYPE_TEXT, 
+        RESPONSETYPE_DOCUMENT,
+        RESPONSETYPE_BLOB,
+        RESPONSETYPE_ARRAYBUFFER

Enums shouldn't use ALL_CAPS.

> * the return type on the .response attribute should by "Any" in the .idl, but I don't know how to do that

Would Object work?

[Eric Seidel (no email)]

Attachment 74065 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6109054

[Chris Rogers]

Created attachment 74421 [details]
Patch

[Chris Rogers]

Alexey, I've addressed your comments, except the one about markChildren().  Can you please give me a few more details of what actual code I should add there - thanks!

I've added two layout tests:

xmlhttprequest-responsetype-arraybuffer.html
Does basic sanity checking on .responseType and .response behavior, exception throwing, etc.
Loads a moderately large binary file and verifies it's able to get a correct ArrayBuffer from it.

xmlhttprequest-responsetype-text.html
This is a simpler test which simply verifies that setting .responseType to "text" and later accessing .response (instead of .responseText) will get the correct text.

* still need to add the actual binary test file for the above test

* still need to add the custom bindings for V8

[WebKit Review Bot]

Attachment 74421 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6237066

[Eric Seidel (no email)]

Attachment 74421 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6190080

[Alexey Proskuryakov]

> Can you please give me a few more details of what actual code I should add there - thanks!

For responseXML, the following needs to be added to JSXMLHttpRequest::markChildren in JSXMLHttpRequestCustom.cpp:

+    if (Document* responseDocument = m_impl->optionalResponseXML())
+        markDOMObjectWrapper(markStack, *Heap::heap(this)->globalData(), responseDocument);

Here, XMLHttpRequest::optionalResponseXML() will be a new function that returns m_responseXML if it's already non-zero, similar to optionalUpload().

The same needs to be added for any object that XMLHttpRequest exposes to bindings as .response.

This of course requires tests that fail without the additional code, but pass with it. Something like:

// ... request an XML resource
req.responseXML.foo = "bar";
gc();
shouldBe("req.responseXML.foo", "'bar'");

[Chris Rogers]

Created attachment 74437 [details]
Patch

[Chris Rogers]

* Thanks Alexey, I've added code in JSXMLHttpRequest::markChildren() to handle the Document from .responseXML as well as Blob and ArrayBuffer.

* added a test as you suggest for garbage collection with .responseXML as well as .response

* added the V8 custom bindings 

* added the binary file used for testing

[Chris Marrin]

I agree with Alexey's earlier comment. I'd really like to see responseType take enumerated constants rather than strings. There should be a default of None which has the same behavior as before: responseText and responseXML return what they always have. In this mode I thing using response results in an exception. The other values (TEXT, DOCUMENT, BLOB AND ARRAYBUFFER) when set in responseType would return the appropriate value in response. In this mode responseText and responseXML would throw an exception. 

This would make the implementation easier and would avoid give authors the most clear guidance on how to use the new functionality.

[Alexey Proskuryakov]

This looks really good to me. Not sure if we want more baking time for discussion, keeping at r? in case we do.

As for string vs. named type discussion - let's do whatever is required to get other browser vendors on board with the new API, this doesn't seem to be a major issue. I haven't been following e-mail discussion in enough detail to tell who had a very strong opinion on that.

+    else if (responseType == "blob") {
+#if ENABLE(XHR_RESPONSE_BLOB)
+        m_responseTypeCode = ResponseTypeBlob;
+#else
+        ec = SYNTAX_ERR;
+#endif

It would be cleaner to just let it fall through to default section.

+    case ResponseTypeBlob:
+        return "blob";

I'm somewhat surprised that this is not inside an #if (ditto for enum values in header). This is not a big deal - if this has made other code significantly more readable in your opinion, let's keep it.

-    bool asBlob() const { return m_asBlob; }
+    bool asBlob() const { return responseTypeCode() == ResponseTypeBlob; }

Our normal naming scheme is for asXXX() and toXXX() functions to return XXX objects.

+responseXML serialized
+<!DOCTYPE doc><doc>
+  <foo xmlns="foobar">One</foo> <x:bar xmlns:x="barfoo">Two</x:bar>
+  <d id="id3">Three</d>
+</doc>

There is a downside to making this test depend on minute details of XML serialization. E.g., it's likely to catch someone off guard when fixing bug 25206. 

I'd love to see tests for what happens when accessing .response when the load is only partially done, and in case the request has been aborted.

> Index: LayoutTests/fast/xmlhttprequest/resources/balls-of-the-orient.aif

We've got tons of resource files to request, why add another huge one? If you intend to use it for testing audio API later, then the location may not be so good.

[Tab Atkins]

(In reply to comment #14)
> As for string vs. named type discussion - let's do whatever is required to get other browser vendors on board with the new API, this doesn't seem to be a major issue. I haven't been following e-mail discussion in enough detail to tell who had a very strong opinion on that.

Jonas Sicking from Moz seemed to like the string version, or at least he didn't object to it, as he used it in his examples at <http://lists.w3.org/Archives/Public/public-webapps/2010OctDec/0363.html>.

[Chris Rogers]

Created attachment 74603 [details]
Patch

[Chris Rogers]

Alexey, thanks for having a look.  I've posted a new patch addressing most of your comments:

(In reply to comment #14)
> This looks really good to me. Not sure if we want more baking time for discussion, keeping at r? in case we do.
> 
> As for string vs. named type discussion - let's do whatever is required to get other browser vendors on board with the new API, this doesn't seem to be a major issue. I haven't been following e-mail discussion in enough detail to tell who had a very strong opinion on that.
> 

I had brought up the issue of integer enum values vs. string on the list, but the only response I got was to use strings.  Jonas Sicking from Mozilla first used string constants in his example:
http://lists.w3.org/Archives/Public/public-webapps/2010OctDec/0336.html



> +    else if (responseType == "blob") {
> +#if ENABLE(XHR_RESPONSE_BLOB)
> +        m_responseTypeCode = ResponseTypeBlob;
> +#else
> +        ec = SYNTAX_ERR;
> +#endif
> 
> It would be cleaner to just let it fall through to default section.

FIXED


> 
> +    case ResponseTypeBlob:
> +        return "blob";
> 
> I'm somewhat surprised that this is not inside an #if (ditto for enum values in header). This is not a big deal - if this has made other code significantly more readable in your opinion, let's keep it.

I think it's a bit more readable as is.



> 
> -    bool asBlob() const { return m_asBlob; }
> +    bool asBlob() const { return responseTypeCode() == ResponseTypeBlob; }
> 
> Our normal naming scheme is for asXXX() and toXXX() functions to return XXX objects.

I didn't add this method.  It was already there to support the 'asBlob' attribute in the .idl file which is in the current proposed spec.  This patch should make it unnecessary to have 'asBlob' but, for now, I left it in with the
behavior unchanged.  I spoke with Michael Nordman earlier about this, and he thought we could probably just remove it, but he's on vacation right now so maybe it's safer to remove
it in another patch?


> 
> +responseXML serialized
> +<!DOCTYPE doc><doc>
> +  <foo xmlns="foobar">One</foo> <x:bar xmlns:x="barfoo">Two</x:bar>
> +  <d id="id3">Three</d>
> +</doc>
> 
> There is a downside to making this test depend on minute details of XML serialization. E.g., it's likely to catch someone off guard when fixing bug 25206. 

I'm happy to remove this part of the test, but I wanted to cover the case where I check for the case when .responseType is set to "document".  Is there a simpler way you can recommend?


> 
> I'd love to see tests for what happens when accessing .response when the load is only partially done, and in case the request has been aborted.

Tests have been added for accessing .response when .responseState != DONE.  This caught a bug in my previous patch, which I also fixed.  My behavior is to 
require that the response be DONE before an ArrayBuffer .response is available (similar to .responseXML).  I think this will cover the 99% use case, and we can always add the capability
to access it progressively as data comes in later on if it's thought to be important.

I also added a new test for abort behavior.


> 
> > Index: LayoutTests/fast/xmlhttprequest/resources/balls-of-the-orient.aif
> 
> We've got tons of resource files to request, why add another huge one? If you intend to use it for testing audio API later, then the location may not be so good.

I understand.  I just wanted to use a file in the xmlhttprequest directory because I was worried that if I referenced a different file (maybe in the media directory) that someone might
change it for a different test and wouldn't realize that my tests also relied on it.  If you think this is not a concern, then I'll change to reference a file in the media directory.

[Alexey Proskuryakov]

Comment on attachment 74603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74603&action=review

> maybe it's safer to remove it in another patch?

Sure, that's fine.

> I'm happy to remove this part of the test, but I wanted to cover the case where I check 
> for the case when .responseType is set to "document".  Is there a simpler way you can recommend?

I'd just check that the returned object's prototype === Document's prototype. Or alternatively, check something like xhr.response.documentElement.tagName.

> If you think this is not a concern, then I'll change to reference a file in the media directory.

This is a slight concern, which we usually ignore. Or you could put the file to top level http/tests/resources directory.

Or you could just load some other file (even the .html test itself, perhaps).

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-text.html:92
> +        if ("responseType" in xhr)
> +            testPassed("responseType property exists.");

A very minor detail - it would be nice to have output in failure case, too. For example, if we refer someone from Mozilla to this test, it would be nice if it didn't hide failures.

[Chris Rogers]

Committed r72626: <http://trac.webkit.org/changeset/72626>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72626 might have broken Qt Linux Release
The following tests are not passing:
fast/xmlhttprequest/xmlhttprequest-responsetype-abort.html
fast/xmlhttprequest/xmlhttprequest-responsetype-arraybuffer.html

[Chris Rogers]

removed the tests for Qt and gtk here since they require ArrayBuffer support:
https://bugs.webkit.org/show_bug.cgi?id=49992