[webkit-reviews] review denied: [Bug 90976] If value for responseType defined as type that not supported, it should not throw an exception in XHR 2 : [Attachment 152971] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 90976: If value for responseType defined as type that not supported, it
should not throw an exception in XHR 2
https://bugs.webkit.org/show_bug.cgi?id=90976

Attachment 152971: Patch
https://bugs.webkit.org/attachment.cgi?id=152971&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152971&action=review


> Source/WebCore/ChangeLog:9
> +	   The spec doesn't say it should throw an exception when a
non-supported type is set, or other browsers does not throw it either.

s/ or / and /
s/ does / do /

> Source/WebCore/xml/XMLHttpRequest.cpp:347
> +    // http://www.w3.org/TR/XMLHttpRequest2/#the-responsetype-attribute
> +    // An attempt to set an invalid type is silently discarded (does not
change the value).

Please remove this comment. It just re-states what the code below does, and
gives a link to a spec anyone working on this code knows about.

> Source/WebCore/xml/XMLHttpRequest.cpp:-357
> -    else
> -	   ec = SYNTAX_ERR;

I think that we should log to Inspector when an unsupported type is used. It's
something that is going to break sites, and would be incredibly hard to debug
without an exception or any diagnostic logging.

>
LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types-expected.
txt:5
> +PASS No exception.

This is not the best way to write a test. Ideally, the output would be
self-explanatory - it's hard to correlate output lines with subtests here.

In this particular case, you can consider two approaches:
1. Use shouldNotThrow() function provided by js-test-pre.js;
2. Put preparation code inside shouldBe, like shouldBe("xhr.responseType =
'text'; xhr.responseType", "'text'");

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types.html:9
> +function setResponseType(type, expected)

This function doesn't just set the response type, so the name is misleading.

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-set-types.html:27

> +setResponseType('arraybuffer', 'arraybuffer');
> +setResponseType('blob', 'blob');

Aren't these under ifdefs? We'll need platform specific results because of
these sub-tests, and they don't really check anything changed in this patch.