WebKit Bugzilla
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
Created attachment 64905 [details] xhr.responseBlob bindings
Created attachment 64915 [details] xhr.responseBlob bindings
Created attachment 65153 [details] xhr.responseBlob bindings
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?
I'm not a reviewer but this change looks good to me.
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; > + }
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.
(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.
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.
I'll fix the changelog entry typo too (forgot to do that).
Created attachment 65351 [details] xhr.responseBlob bindings
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 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
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
(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?)
> 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 :)
Created attachment 65458 [details] xhr.responseBlob bindings #include <wtf/UnusedParam.h>
(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 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 on attachment 65458 [details] xhr.responseBlob bindings Clearing flags on attachment: 65458 Committed r66074: <http://trac.webkit.org/changeset/66074>
All reviewed patches have been landed. Closing bug.
Hmmm... reverted? https://bugs.webkit.org/show_bug.cgi?id=44660
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
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 on attachment 65607 [details] xhr.responseBlob bindings Clearing flags on attachment: 65607 Committed r66243: <http://trac.webkit.org/changeset/66243>