[webkit-reviews] review denied: [Bug 72154] Synchronous XHR in window context should not support new XHR responseTypes : [Attachment 120361] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jarred Nicholls
<jarred at webkit.org>'s request for review:
Bug 72154: Synchronous XHR in window context should not support new XHR
responseTypes
https://bugs.webkit.org/show_bug.cgi?id=72154

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

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


> If between you and I we're good with that idea, I can modify the patch to
only reject HTTP requests.

Thanks, let's limit this to http and https then. Comments about the current
version below.

> Source/WebCore/xml/XMLHttpRequest.cpp:286
> -    if (m_state != OPENED || m_loader) {
> +    if (m_state > OPENED || m_loader) {

This change is not explained in ChangeLog, and doesn't match the spec (one
should also be able to set this property in HEADERS_RECEIVED state). Please
address this, and add a separate test case.

> Source/WebCore/xml/XMLHttpRequest.cpp:293
> +	   ec = INVALID_ACCESS_ERR;
> +	   return;

I think that we owe developers a console message here.

> Source/WebCore/xml/XMLHttpRequest.cpp:-418
> -    m_responseTypeCode = ResponseTypeDefault;

In fact, this should not be only tested and mentioned in ChangeLog, but be done
in a separate bug.

> Source/WebCore/xml/XMLHttpRequest.cpp:447
> +    // Synchronous requests from a window context should not be able to use
responseType per W3C spec.

You have a comment here, but not in setResponseType(). The comment could say
more about "why", e.g. "Newer functionality is not available to synchronous
requests in window contexts, as a spec-mandated attempt to discourage
synchronous XHR use."

> Source/WebCore/xml/XMLHttpRequest.cpp:449
> +    if (!async && scriptExecutionContext()->isDocument()
> +	   && m_responseTypeCode != ResponseTypeDefault) {

I wouldn't have wrapped this line.

> Source/WebCore/xml/XMLHttpRequest.cpp:451
> +	   ec = INVALID_ACCESS_ERR;
> +	   return;

I think that we owe developers a console message here.

>
LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html:6

> +	   description('This tests that the XMLHttpRequest responseType
attribute is not usable with synchronous requests.');

We can still read it, so I would say "not modifiable".