[webkit-reviews] review denied: [Bug 136938] XMLHttpRequest should have a responseURL attribute (added in recent XHR specification drafts) : [Attachment 238590] Patch-Updated-Review

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Shivakumar J M
<shiva.jm at samsung.com>'s request for review:
Bug 136938: XMLHttpRequest should have a responseURL attribute (added in recent
XHR specification drafts)
https://bugs.webkit.org/show_bug.cgi?id=136938

Attachment 238590: Patch-Updated-Review
https://bugs.webkit.org/attachment.cgi?id=238590&action=review

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


Thank you for updating the patch, and sorry that it took me long to review. I
think that test coverage still needs to be substantially extended, and another
round of review would help.

You did not add any tests for fragment in URL. Please add them. I suspect that
a code change may be needed to make that work correctly.

Please add a test for what happens for a cross-origin redirect - both
successful an unsuccessful cases. Please add a test for redirecting to a URL
with credentials, too.

It is not necessary to add all test cases in in file - in fact, it is often
preferable to split them, because then a file name serves an an explanation of
what exactly is being tested, and because it's easier to debug small tests when
something breaks.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:13
> +    return new Promise(function(resolve, reject) {
> +    var req = new XMLHttpRequest();

Style nitpick: code inside the promise handler should be indented.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:14
> +    req.open('GET', url);

We need to test what responseURL does in UNSENT state, i.e. before calling
open().

We also need to test what it does immediately after calling open().

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:21
> +    req.send();

We need to test what responseURL does after calling send(). This doesn't change
XHR state, so onreadystatechange is not called.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:28
> +window.responseURL = req.responseURL;
> +shouldBeEqualToString('responseURL',
'http://127.0.0.1:8000/xmlhttprequest/resources/reply.txt');
> +return runTest('resources/redirect_methods.php?url=reply.xml', 'document');

Please indent function bodies.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:40
> +return runTest('resources/network-simulator.php?command=connect/', 'text');

What specifically does this test? Changing network simulator state here will
probably not break anything, but it's surprising anyway.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL.html:46
> + testFailed(String(reason));

Please use 4 spaces to indent.