[webkit-reviews] review denied: [Bug 99983] XMLHttpRequest Content-Type should be taken from Blob type : [Attachment 170880] Patch 2

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Alexander Shalamov
<alexander.shalamov at gmail.com>'s request for review:
Bug 99983: XMLHttpRequest Content-Type should be taken from Blob type
https://bugs.webkit.org/show_bug.cgi?id=99983

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

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


Looks good, r- for failing test.

> LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-expected.txt:9
> +PASS Sync test: Blob Content-Type: "invalid/type\r\ncharset;=koi8-r". Sent
Content-type: ""

I'd have added separate tests for CR, LF, CRLF, and for Unicode newlines. Also,
for a boundary - unexpected things may happen if the request becomes multipart.
Also, quote marks.

Please consider any tests you can think of - as mentioned before, we're adding
a new attack vector, and should be super vigilant.

> LayoutTests/http/tests/xmlhttprequest/post-blob-content-type-expected.txt:18
> +PASS Async test: Should fail.

So is it a PASS or a FAIL then? :)

I would prefer the test split into multiple files. That way, long-term
maintenance is much easier. A series of shouldBe is best when we are checking a
long list of very similar cases, but these are quite different.

You could make test machinery simpler and easier to understand.

Filename of a separate case is a very prominent place for a brief description,
so if a test fails, you already have an idea of what went wrong simply by
looking at its name. It's much easier to get rid of these "pass must fail"
mysteries, too.

> LayoutTests/http/tests/xmlhttprequest/post-blob-content-type.html:4
> +  <script src="../../js-test-resources/js-test-pre.js"></script>

No need for the relative path here - as this is an HTTP test, you can start
form the root. Not a big deal - will just make it easier to move the test into
a different directory if needed, for example.

> LayoutTests/http/tests/xmlhttprequest/post-blob-content-type.html:116
> +	   if (async) xhr.onloadend = reportResult;

I usually don't object against non-WebKit coding style in tests, but this one
has logic complicated enough that I feel tempted to.