Bug 44133 - Implement an XHR.responseBlob accessor
Summary: Implement an XHR.responseBlob accessor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on: 44660
Blocks: 44721
  Show dependency treegraph
 
Reported: 2010-08-17 14:51 PDT by Michael Nordman
Modified: 2010-08-27 12:43 PDT (History)
6 users (show)

See Also:


Attachments
xhr.responseBlob bindings (13.33 KB, patch)
2010-08-19 15:52 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
xhr.responseBlob bindings (13.33 KB, patch)
2010-08-19 17:18 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
xhr.responseBlob bindings (13.37 KB, patch)
2010-08-23 12:43 PDT, Michael Nordman
levin: review-
Details | Formatted Diff | Diff
xhr.responseBlob bindings (14.00 KB, patch)
2010-08-24 17:56 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
xhr.responseBlob bindings (14.00 KB, patch)
2010-08-24 18:01 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
xhr.responseBlob bindings (14.06 KB, patch)
2010-08-25 11:30 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
xhr.responseBlob bindings (14.25 KB, patch)
2010-08-25 11:57 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
xhr.responseBlob bindings (14.57 KB, patch)
2010-08-26 13:16 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2010-08-17 14:51:14 PDT
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
Comment 1 Michael Nordman 2010-08-19 15:52:04 PDT
Created attachment 64905 [details]
xhr.responseBlob bindings
Comment 2 Michael Nordman 2010-08-19 17:18:19 PDT
Created attachment 64915 [details]
xhr.responseBlob bindings
Comment 3 Michael Nordman 2010-08-23 12:43:59 PDT
Created attachment 65153 [details]
xhr.responseBlob bindings
Comment 4 Michael Nordman 2010-08-23 16:31:58 PDT
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?
Comment 5 Kinuko Yasuda 2010-08-23 23:30:40 PDT
I'm not a reviewer but this change looks good to me.
Comment 6 David Levin 2010-08-24 14:58:32 PDT
Comment on attachment 65153 [details]
xhr.responseBlob bindings

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 65820)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,36 @@
> +2010-08-19  Michael Nordman  <michaeln@google.com>
> +
> +        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;
> +    }
Comment 7 Michael Nordman 2010-08-24 15:22:42 PDT
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.
Comment 8 David Levin 2010-08-24 16:09:43 PDT
(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.
Comment 9 Michael Nordman 2010-08-24 17:56:31 PDT
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.
Comment 10 Michael Nordman 2010-08-24 17:58:28 PDT
I'll fix the changelog entry typo too (forgot to do that).
Comment 11 Michael Nordman 2010-08-24 18:01:03 PDT
Created attachment 65351 [details]
xhr.responseBlob bindings
Comment 12 David Levin 2010-08-24 18:08:50 PDT
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
Comment 13 WebKit Commit Bot 2010-08-25 05:24:37 PDT
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
Comment 14 Michael Nordman 2010-08-25 11:30:48 PDT
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
Comment 15 David Levin 2010-08-25 11:37:37 PDT
(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?)
Comment 16 Michael Nordman 2010-08-25 11:40:54 PDT
> 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 :)
Comment 17 Michael Nordman 2010-08-25 11:57:09 PDT
Created attachment 65458 [details]
xhr.responseBlob bindings

#include <wtf/UnusedParam.h>
Comment 18 David Levin 2010-08-25 11:59:17 PDT
(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.
Comment 19 Eric Seidel (no email) 2010-08-25 17:54:30 PDT
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.
Comment 20 WebKit Commit Bot 2010-08-25 19:54:27 PDT
Comment on attachment 65458 [details]
xhr.responseBlob bindings

Clearing flags on attachment: 65458

Committed r66074: <http://trac.webkit.org/changeset/66074>
Comment 21 WebKit Commit Bot 2010-08-25 19:54:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Michael Nordman 2010-08-26 12:44:25 PDT
Hmmm... reverted?
https://bugs.webkit.org/show_bug.cgi?id=44660
Comment 23 Michael Nordman 2010-08-26 12:47:41 PDT
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
Comment 24 Michael Nordman 2010-08-26 13:16:24 PDT
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
Comment 25 WebKit Commit Bot 2010-08-27 12:42:58 PDT
Comment on attachment 65607 [details]
xhr.responseBlob bindings

Clearing flags on attachment: 65607

Committed r66243: <http://trac.webkit.org/changeset/66243>
Comment 26 WebKit Commit Bot 2010-08-27 12:43:05 PDT
All reviewed patches have been landed.  Closing bug.