[Webkit-unassigned] [Bug 136938] The XMLHttpRequest should suppourt method responseURL As per latest Spec

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #3 from Darin Adler <darin at apple.com>  2014-09-19 09:05:27 PST ---
(From update of attachment 238368)
View in context: https://bugs.webkit.org/attachment.cgi?id=238368&action=review

Patch is OK, but not quite right, and we need a bit more testing.

> Source/WebCore/xml/XMLHttpRequest.cpp:343
> +    if (!m_response.url().string().isNull())
> +        return m_response.url().string();
> +
> +    return "";

This check for null isn’t needed because the JavaScript binding layer should already turn null strings into empty strings unless we use keywords to tell it not to do so. It’s also possibly inefficient to call m_response.url().string() twice; the compiler might not be able to optimize the common subexpression. And further, the correct efficient idiom for an empty string is emptyString(), not "".

We need test coverage for this. The test doesn’t currently seem to cover the null case and it needs to.

> Source/WebCore/xml/XMLHttpRequest.h:135
> +    String responseURL();

I think this should be a const member function. Any reason to have it not be one?

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseURL-expected.txt:9
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"
> +PASS req.responseURL is "http://127.0.0.1:8000/xmlhttprequest/resources/small-chunks.cgi"

This test is inadequate. In this case the response URL is exactly the same as the request URL; the primary feature of response URL is to expose cases where the two aren't the same, and we need to make a test that covers that. The cases where a response URL might be different from the request URL would at least include things like redirects. I’d like to understand the complete set of cases where the response URL could be different from the request and have coverage for those. Our HTTP tests are set up to be able to deal with things like redirects.

We also need a test for the case where the response URL is null to test our code that turns it into the empty string rather than a JavaScript null.

I also don’t understand why we get exactly 5 pieces. Is that reliable or could this test be flaky?

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