[webkit-reviews] review denied: [Bug 72154] Synchronous XHR in window context should not support new XHR responseTypes for HTTP(S) requests : [Attachment 120434] 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 for HTTP(S) requests
https://bugs.webkit.org/show_bug.cgi?id=72154

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

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


r- because of inappropriate use of reportUnsafeUsage. I suggest just using
addConsoleMessage directly (you can consider abstracting it out when adding the
second check later).

> Source/WebCore/xml/XMLHttpRequest.cpp:301
> +    // attempt to discourage synchronous XHR use. responseType is one such
function.

responseType is not a function :)

> Source/WebCore/xml/XMLHttpRequest.cpp:305
> +	   reportUnsafeUsage(scriptExecutionContext(), "XMLHttpRequest cannot
set responseType for synchronous HTTP(S) requests made from the window
context.");

This is not unsafe usage, so calling a function named "reportUnsafeUsage" is
not appropriate.

The comment appears OK, although grammar seems slightly off (it's not
XMLHttpRequest who does the setting). I'd have said
"XMLHttpRequest.responseType cannot be changed ... from a window context".