Bug 44133

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>

All reviewed patches have been landed. Closing bug.