[webkit-reviews] review denied: [Bug 44133] Implement an XHR.responseBlob accessor : [Attachment 65153] xhr.responseBlob bindings

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 44133: Implement an XHR.responseBlob accessor
https://bugs.webkit.org/show_bug.cgi?id=44133

Attachment 65153: xhr.responseBlob bindings
https://bugs.webkit.org/attachment.cgi?id=65153&action=review

------- Additional Comments from David Levin <levin at chromium.org>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 65820)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,36 @@
> +2010-08-19  Michael Nordman	<michaeln at 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;
> +    }