[Webkit-unassigned] [Bug 136938] XMLHttpRequest should have a responseURL attribute (added in recent XHR specification drafts)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136938


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #238590|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #12 from Alexey Proskuryakov <ap at webkit.org>  2014-10-09 11:20:12 PST ---
(From update of attachment 238590)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.