Bug 44133 – Implement an XHR.responseBlob accessor

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Michael Nordman]

Implement an XHR.responseBlob accessor to get an opaque reference to the response data as a blob.

Two new additions for the XHR interface in support of this. This is not solidified yet, still pending some discussion on the public lists.

boolean attribute asBlob;
// Prepares the XHR to make the response available as a blob object.
// Defaults to false, must be set after calling open() and
// prior to calling send(). Gets reset upon subsequent calls to open().
// Throws INVALID_STATE_ERR if set at an invalid time. Maybe read at
// anytime without exception.

Blob attribute responseBlob;
// Returns a blob the contains the response body.
// Only valid to access when asBlob is true and when the request is in
// a terminal state. Throws INVALID_STATE_ERR if accessed at an
// invalid time.

When asBlob is true, the other response accessors (responseText, resonseXML, responseBody) throw INVALID_STATE_ERR if accessed.

We're making this "modal" for the benefit of the browser vendors, to make it easier for them to know how to handle the response data as it comes in. With a priori knowlege that the data need never be made available thru the responseText attribute, life is a little easier for them/us.

Also see http://code.google.com/p/chromium/issues/detail?id=52486

[Michael Nordman]

Created attachment 64905 [details]
xhr.responseBlob bindings

[Michael Nordman]

Created attachment 64915 [details]
xhr.responseBlob bindings

[Michael Nordman]

Created attachment 65153 [details]
xhr.responseBlob bindings

[Michael Nordman]

Comment on attachment 65153 [details]
xhr.responseBlob bindings

WebCore/xml/XMLHttpRequest.cpp:347
 +      if (!RuntimeEnabledFeatures::responseBlobEnabled()) {
This check isn't needed for V8 since the test occurs in the bindings layer. Is testing here an appropriate way of handling this for JSC?

[Kinuko Yasuda]

I'm not a reviewer but this change looks good to me.

[David Levin]

Comment on attachment 65153 [details]
xhr.responseBlob bindings

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 65820)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,36 @@
> [phone]  Michael Nordman  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=44133
> +        IDL bindings for XmlHttpRequest.responseBlob support, doesn't do anything yet.
> +        Adds two new attributes, asBlob and responseBlob.
> +        Runtime disabled by default, also behind the ENABLE_BLOB compile time guard.

I'm a bit concerned by using the same define. It is only runtime disabled for v8 but now people who want to implement blob's have this new work to finish to turn on the define.



> +        
> +        No new tests, just adding some stubs.
> +
> +        * bindings/generic/RuntimeEnabledFeatures.cpp:
> +        * bindings/generic/RuntimeEnabledFeatures.h:
> +        (WebCore::RuntimeEnabledFeatures::setResponseBlobEnabled):
> +        (WebCore::RuntimeEnabledFeatures::responseBlobEnabled):
> +        (WebCore::RuntimeEnabledFeatures::asBlobEnabled):
> +        * bindings/js/JSXMLHttpRequestCustom.cpp:
> +        (WebCore::JSXMLHttpRequest::responseText): Changed to allow an exceptional return path

Please add "."

> +        * bindings/v8/custom/V8XMLHttpRequestCustom.cpp:
> +        (WebCore::V8XMLHttpRequest::responseTextAccessorGetter): Changed to allow an exceptional return path
> +        * xml/XMLHttpRequest.cpp:
> +        (WebCore::XMLHttpRequest::responseText): Changed to raise an exception when accessed with asBlob set to true
> +        (WebCore::XMLHttpRequest::responseXML): Changed to raise an exception when accessed with asBlob set to true

"Ditto." works instead of repeating the same text.

> +        (WebCore::XMLHttpRequest::responseBlob): Added stub method, returns 0 for now
> +        (WebCore::XMLHttpRequest::setAsBlob): Sets the asBlob attribute, raises exception if called at an inapropiate time
Typo: inapropiate



> Index: WebCore/bindings/generic/RuntimeEnabledFeatures.h
>      static bool speechInputEnabled() { return isSpeechInputEnabled; }
>      static bool speechEnabled() { return isSpeechInputEnabled; }
>  
> +    static void setResponseBlobEnabled(bool isEnabled) { isResponseBlobEnabled = isEnabled; }
> +    static bool responseBlobEnabled() { return isResponseBlobEnabled; }
> +    static bool asBlobEnabled()  { return isResponseBlobEnabled; }

In this context, asBlob really doesn't stand on its own. Also why are these two separate bools?   Oh I see, I guess the tooling needs this name and the separate Enabled flags.

Why not name the set as setBlobInXMlHttpRequestEnabled because if I just looked at setResponseBlobEnabled alone, I'm not sure that I would know what it does.


> Index: WebCore/bindings/js/JSXMLHttpRequestCustom.cpp
> ===================================================================
> +    ExceptionCode ec = 0;
> +    const ScriptString& text = impl()->responseText(ec);

const ScriptString& feels iffy. It seems to rely on an implementation detail that the underlying script wasn't created just to return it.

Copying these string is very cheap, so just make it a ScriptString.



> Index: WebCore/bindings/v8/custom/V8XMLHttpRequestCustom.cpp
> +    ExceptionCode ec = 0;
> +    const ScriptString& text = xmlHttpRequest->responseText(ec);

Ditto.


> Index: WebCore/xml/XMLHttpRequest.cpp
> ===================================================================
> +#if ENABLE(BLOB)
> +void XMLHttpRequest::setAsBlob(bool value, ExceptionCode& ec)
> +{
> +    if (!RuntimeEnabledFeatures::responseBlobEnabled()) {

How does it get here?

> +        ec = NOT_SUPPORTED_ERR;
> +        return;
> +    }

[Michael Nordman]

Thnx for looking!

(In reply to comment #6)
> > +        Runtime disabled by default, also behind the ENABLE_BLOB compile time guard.
> 
> I'm a bit concerned by using the same define. It is only runtime disabled for v8 but now
> people who want to implement blob's have this new work to finish to turn on the define.

Ok... ENABLE(XML_HTTP_REQUEST_RESPONSE_BLOB) or ENABLE(XHR_RESPONSE_BLOB)... any preference?

I guess i should put my other changes (in the works) behind this flag too.

> Please add "."

Will do

> Typo: inapropiate

Ditto

> > +    static void setResponseBlobEnabled(bool isEnabled) { isResponseBlobEnabled = isEnabled; }
> > +    static bool responseBlobEnabled() { return isResponseBlobEnabled; }
> > +    static bool asBlobEnabled()  { return isResponseBlobEnabled; }
> 
> In this context, asBlob really doesn't stand on its own. Also why are these two separate bools?
> Oh I see, I guess the tooling needs this name and the separate Enabled flags.

Yes, because of how the generated bindings work.

> Why not name the set as setBlobInXMlHttpRequestEnabled because if I just looked at setResponseBlobEnabled
> alone, I'm not sure that I would know what it does.

Sure... how about...
void setXmlHttpRequestResponseBlobEnabled(bool)
bool responseBlobEnabled();
bool asBlobEnabled();


> const ScriptString& feels iffy. It seems to rely on an implementation detail that the underlying script wasn't created just to return it.
> 
> Copying these string is very cheap, so just make it a ScriptString.

Not sure about this one. I had no idea that copying this type of string is cheap. Changing it to make that copy feels like it relies on that implementation detail that copies are cheap to make.

That method of XmlHttpReqeust is defined to return a const ref
const ScriptString& responseText(ExceptionCode&) const;
As coded, all i'm relying on is that public interface.

I'd prefer to leave this as is.


> > +    if (!RuntimeEnabledFeatures::responseBlobEnabled()) {
> 
> How does it get here?

See the previous comment in the review log. Looks like JSC bindings don't handle runtime disablement like v8 bindings do. That's why I was asking about how to handle this difference.

[David Levin]

(In reply to comment #7)
> Thnx for looking!
> 
> (In reply to comment #6)
> > > +        Runtime disabled by default, also behind the ENABLE_BLOB compile time guard.
> > 
> > I'm a bit concerned by using the same define. It is only runtime disabled for v8 but now
> > people who want to implement blob's have this new work to finish to turn on the define.
> 
> Ok... ENABLE(XML_HTTP_REQUEST_RESPONSE_BLOB) or ENABLE(XHR_RESPONSE_BLOB)... any preference?

ENABLE(XHR_RESPONSE_BLOB) 


> Sure... how about...
> void setXmlHttpRequestResponseBlobEnabled(bool)
> bool responseBlobEnabled();
> bool asBlobEnabled();

Ok.


> > const ScriptString& feels iffy. It seems to rely on an implementation detail that the underlying script wasn't created just to return it.
> I'd prefer to leave this as is.

ok.

> 
> 
> > > +    if (!RuntimeEnabledFeatures::responseBlobEnabled()) {
> See the previous comment in the review log. Looks like JSC bindings don't handle runtime disablement like v8 bindings do. That's why I was asking about how to handle this difference.

As discussed.

[Michael Nordman]

Created attachment 65348 [details]
xhr.responseBlob bindings

Switched to using ENABLE(XHR_RESPONSE_BLOB) instead of ENABLE(BLOB).
Defined that only for the chromium port in features.gypi
And other changes as we discussed.

[Michael Nordman]

I'll fix the changelog entry typo too (forgot to do that).

[Michael Nordman]

Created attachment 65351 [details]
xhr.responseBlob bindings

[David Levin]

Comment on attachment 65351 [details]
xhr.responseBlob bindings

(In a future patch) Please consider hooking up your define for other platforms as well like this: http://trac.webkit.org/changeset/61545

[WebKit Commit Bot]

Comment on attachment 65351 [details]
xhr.responseBlob bindings

Rejecting patch 65351 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
 setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/XMLHttpRequest.o /Users/eseidel/Projects/CommitQueue/WebCore/xml/XMLHttpRequest.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/3735585

[Michael Nordman]

Created attachment 65449 [details]
xhr.responseBlob bindings

Ooops... use UNUSED_PARAM(ec) in the responseText(ExceptionCode& ec) and responseXML(ExceptionCode& ec) methods when XHR_RESPONSE_BLOB is not enabled.

-const ScriptString& XMLHttpRequest::responseText() const
+const ScriptString& XMLHttpRequest::responseText(ExceptionCode& ec) const
 {
+#if ENABLE(XHR_RESPONSE_BLOB)
+    if (m_asBlob)
+        ec = INVALID_STATE_ERR;
+#else
+    UNUSED_PARAM(ec);
+#endif
     return m_responseText;
 }
 
-Document* XMLHttpRequest::responseXML() const
+Document* XMLHttpRequest::responseXML(ExceptionCode& ec) const
 {
+#if ENABLE(XHR_RESPONSE_BLOB)
+    if (m_asBlob) {
+        ec = INVALID_STATE_ERR;
+        return 0;
+    }
+#else
+    UNUSED_PARAM(ec);
+#endif

[David Levin]

(In reply to comment #14)
> Created an attachment (id=65449) [details]
> xhr.responseBlob bindings
> 
> Ooops... use UNUSED_PARAM(ec) in the responseText(ExceptionCode& ec) and responseXML(ExceptionCode& ec) methods when XHR_RESPONSE_BLOB is not enabled.

You should include <wtf/unused.h> (didn't r- since you didn't r?)

[Michael Nordman]

> You should include <wtf/unused.h> (didn't r- since you didn't r?)

Yup, noticed that too, that's why i didn't r? it :)

[Michael Nordman]

Created attachment 65458 [details]
xhr.responseBlob bindings

#include <wtf/UnusedParam.h>

[David Levin]

(In reply to comment #16)
> > You should include <wtf/unused.h> (didn't r- since you didn't r?)
> 
> Yup, noticed that too, that's why i didn't r? it :)

fwiw, you can also obsolete a patch.

[Eric Seidel (no email)]

Comment on attachment 65351 [details]
xhr.responseBlob bindings

Cleared David Levin's review+ from obsolete attachment 65351 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 65458 [details]
xhr.responseBlob bindings

Clearing flags on attachment: 65458

Committed r66074: <http://trac.webkit.org/changeset/66074>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

Hmmm... reverted?
https://bugs.webkit.org/show_bug.cgi?id=44660

[Michael Nordman]

From Yuzo...

I've rolled out your change WebKit r66074 because it causes the following error at Chromium canaries:
Webkit (webkit.org)
Webkit Linux (webkit.org)

--- /b/slave/webkit-rel-linux-webkit-org/build/src/webkit/Release/../../../layout-test-results/http/tests/xmlhttprequest/zero-length-response-sync-expected.txt 
+++ /b/slave/webkit-rel-linux-webkit-org/build/src/webkit/Release/../../../layout-test-results/http/tests/xmlhttprequest/zero-length-response-sync-actual.txt 
@@ -1,7 +1,7 @@
 Test for bug 5924 - zero-length responses to XMLHTTPRequest mishandled.
 
 after creation: Uninitialized
-ResponseText: ""
+Exception getting responseText: INVALID_STATE_ERR: DOM Exception 11
 after setting onreadystatechange: Uninitialized
 after open(): Open
 after overrideMimeType(): Open

--- c:\b\slave\webkit-rel-webkit-org\build\src\webkit\Release\..\../../layout-test-results\http/tests/xmlhttprequest/zero-length-response-sync-expected.txt 
+++ c:\b\slave\webkit-rel-webkit-org\build\src\webkit\Release\..\../../layout-test-results\http/tests/xmlhttprequest/zero-length-response-sync-actual.txt 
@@ -1,7 +1,7 @@
 Test for bug 5924 - zero-length responses to XMLHTTPRequest mishandled.
 
 after creation: Uninitialized
-ResponseText: ""
+Exception getting responseText: INVALID_STATE_ERR: DOM Exception 11
 after setting onreadystatechange: Uninitialized
 after open(): Open
 after overrideMimeType(): Open

[Michael Nordman]

Created attachment 65607 [details]
xhr.responseBlob bindings

Ok... let's try this again this time with the new m_asBlob data member in the initializer list and initted to false.

@@ -171,6 +172,9 @@ XMLHttpRequest::XMLHttpRequest(ScriptExe
     : ActiveDOMObject(context, this)
     , m_async(true)
     , m_includeCredentials(false)
+#if ENABLE(XHR_RESPONSE_BLOB)
+    , m_asBlob(false)
+#endif
     , m_state(UNSENT)
     , m_responseText("")
     , m_createdDocument(false)


And another small change to conditionally define the runtime enabled flag

+#if ENABLE(XHR_RESPONSE_BLOB)
+    static bool isXHRResponseBlobEnabled;
+#endif

[WebKit Commit Bot]

Comment on attachment 65607 [details]
xhr.responseBlob bindings

Clearing flags on attachment: 65607

Committed r66243: <http://trac.webkit.org/changeset/66243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.