[webkit-reviews] review denied: [Bug 136938] The XMLHttpRequest should suppourt method responseURL As per latest Spec : [Attachment 238368] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Shivakumar J M
<shiva.jm at samsung.com>'s request for review:
Bug 136938: The XMLHttpRequest should suppourt method responseURL As per latest
Spec
https://bugs.webkit.org/show_bug.cgi?id=136938

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

------- Additional Comments from Darin Adler <darin at apple.com>
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?