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

[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 239793: Patch-Updated-Review5
https://bugs.webkit.org/attachment.cgi?id=239793&action=review

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


r- for the fragment issue.

I also looked over test syntax and output but didn't deeply think about test
semantics yet.

> LayoutTests/http/tests/xmlhttprequest/basic-auth-expected.txt:4
> +TEST COMPLETE
> +PASS req.responseURL is
"http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=a
sync"

The test should only say "TEST COMPLETE" once it's actually complete.

To achieve this, you should switch from waitUntilDone/notifyDone to
jsTestIsAsync=true/finishJSTest().

> LayoutTests/http/tests/xmlhttprequest/basic-auth-expected.txt:10
> +PASS req.responseURL is
"http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=a
sync2"
> +PASS req.responseURL is
"http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=a
sync3"
> +PASS req.responseURL is
"http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=a
sync4"
> +PASS req.responseURL is
"http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=a
sync5"
> +PASS req.responseURL is
"http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=a
sync6"
> +PASS req.responseURL is
"http://127.0.0.1:8000/xmlhttprequest/resources/basic-auth/basic-auth.php?uid=a
sync7"

This output is out of order. The test should use a single mechanism for output
to avoid that.