Implemented XMLHttpRequest.responseURL #9518

shinglyu · 2016-02-03T09:36:16Z

Resolves #8830

KiChjang · 2016-02-03T09:45:23Z

Hi @shinglyu! Does the problem you mentioned in the issue still occur?

jdm · 2016-02-03T13:50:07Z

tests/wpt/metadata/MANIFEST.json

+            "url": "/XMLHttpRequest/responseurl.html"
+          }
+        ],
+        "html/webappapis/scripting/events/event-handler-onresize.html": [


The other entries here should be removed.

jdm · 2016-02-03T13:54:00Z

tests/wpt/metadata/XMLHttpRequest/responsexml-document-properties.htm.ini

@@ -3,12 +3,6 @@
  [domain]
    expected: FAIL

-  [URL]


Huh, I'm confused why this test result changed. The test doesn't use responseURL at all.

jdm · 2016-02-03T13:55:01Z

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html

+        assert_equals(client.responseURL, "")
+        client.open("GET", "foo.html", false)
+        client.send()
+        assert_equals(client.responseURL, "http://web-platform.test:8000/XMLHttpRequest/foo.html")


This should use http://mxr.mozilla.org/servo/source/tests/wpt/web-platform-tests/XMLHttpRequest/responsexml-document-properties.htm?force=1#18 so it will work on other hosts as well.

jdm · 2016-02-03T13:56:38Z

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html

+  <body>
+    <div id="log"></div>
+    <script>
+      test(function() {


We should also have a test that uses an HTTP redirect (resources/redirect.py) and ensures that responseURL shows the post-redirect URL.

jdm · 2016-02-03T13:57:17Z

components/script/dom/xmlhttprequest.rs

@@ -901,6 +901,7 @@ impl XMLHttpRequest {

                // Subsubsteps 5-7
                self.send_flag.set(false);
+                *self.response_url.borrow_mut() = self.request_url.borrow().clone().unwrap().serialize_no_fragment();


self.request_url.borrow().as_ref().unwrap().serialize_no_fragment() will avoid the clone.

jdm · 2016-02-03T13:57:57Z

@bors-servo: try

bors-servo · 2016-02-03T13:57:58Z

⌛ Trying commit 9552064 with merge 19dded5...


        Auto merge of #9518 - shinglyu:responseurl, r=<try>

Implemented XMLHttpRequest.responseURL Resolves #8830  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9518)

bors-servo · 2016-02-03T14:13:23Z

💔 Test failed - linux-dev

shinglyu · 2016-02-04T05:49:28Z

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html, line 12 [r1] (raw file):
I added the test, but it seems I didn't handle the redirection correctly in my code. Let me fix that in the following commits.

Comments from the review on Reviewable.io

shinglyu · 2016-02-04T06:11:19Z

@KiChjang : Yes I still have the issue, so I run the full XMLHttpRequest suite everytime.

bors-servo · 2016-02-05T03:45:37Z

☔ The latest upstream changes (presumably #9521) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-02-08T20:05:16Z

Aha, I figured out why the responseXML tests started passing - we use self.ResponseURL() in the code that creates a new Document that's called from ResponseXML.
-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 4 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from the review on Reviewable.io

shinglyu · 2016-02-15T06:37:38Z

I changed the redirect test to redirect to resources/image.gif, but seem like the responseURL doesn't reflect the redirected URL. I'm still trying to figure out how redirection is handled in XHR, I must have missed something in the spec.

jdm · 2016-02-15T06:45:28Z

Redirections are handled entirely http_loader.rs, so the XHR only sees the final post-redirection Metadata value in headers_available.

KiChjang · 2016-03-21T04:04:46Z

@shinglyu Almost there! Do you need help with rebasing?

shinglyu · 2016-03-21T04:10:55Z

Errrrrr! I found out that this computer's servo clone is a very old one, which has the web-platform-test subfolder as a git submodule. I'll go back and use my laptop to do the rebase, it should have the correct version.

shinglyu · 2016-03-21T04:17:44Z

I though I could use my old desktop computer as my build machine, so I can free my laptop for development, but it turns out the Servo repository in that machine is too old. :'(

jdm · 2016-03-21T04:32:13Z

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html

+        client.send()
+
+        expected = location.href.replace(/[^/]*$/, "resources/" + target)
+        assert_not_equals(client.status, 404)


I left a review comment about making this assert the actual status we're looking for; could you address that?

jdm · 2016-03-21T04:33:18Z

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html

+        expected = location.href.replace(/[^/]*$/, 'foo.html')
+        assert_equals(client.status, 404)
+        assert_equals(client.responseURL, expected)
+      })


Let's add a description for this test (, "404 response has proper responseURL")).

jdm · 2016-03-21T04:33:56Z

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html

+        expected = location.href.replace(/[^/]*$/, "resources/" + target)
+        assert_not_equals(client.status, 404)
+        assert_equals(client.responseURL, expected)
+      })


Description: , "Redirected response has proper responseURL")

jdm · 2016-03-21T04:34:28Z

Sorry, just noticed a couple final issues. Almost there!

jdm · 2016-03-22T03:17:09Z

Sorry, ambiguous wording on my part previously.
-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r9, 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html, line 22 [r9] (raw file):
Sorry, I meant that it should be an argument to the test() function, not assert_equals.

tests/wpt/web-platform-tests/XMLHttpRequest/responseurl.html, line 34 [r9] (raw file):
Same as above.

Comments from the review on Reviewable.io


        Bug 8830 - Implemented XMLHttpRequest.responseURL

jdm · 2016-03-22T18:42:55Z

@bors-servo: r+
Whee! Thanks for finishing this up :)

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

bors-servo · 2016-03-22T18:42:56Z

📌 Commit f55b076 has been approved by jdm

bors-servo · 2016-03-22T18:42:58Z

⌛ Testing commit f55b076 with merge 50193e9...


        Auto merge of #9518 - shinglyu:responseurl, r=jdm

Implemented XMLHttpRequest.responseURL Resolves #8830  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9518)

bors-servo · 2016-03-22T19:34:14Z

☀️ Test successful - android, arm32, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor

highfive added the S-awaiting-review label Feb 3, 2016

jdm reviewed Feb 3, 2016
View changes

tests/wpt/metadata/MANIFEST.json Outdated

"url": "/XMLHttpRequest/responseurl.html"

}

],

"html/webappapis/scripting/events/event-handler-onresize.html": [

This comment has been minimized.

Sign in to view

jdm Feb 3, 2016
Member

The other entries here should be removed.

jdm changed the title ~~Implemented XMLHttpRequest.requestURL~~ Implemented XMLHttpRequest.responseURL Feb 3, 2016

jdm reviewed Feb 3, 2016
View changes

tests/wpt/metadata/XMLHttpRequest/responsexml-document-properties.htm.ini

@@ -3,12 +3,6 @@

[domain]

expected: FAIL

[URL]

This comment has been minimized.

Sign in to view

jdm Feb 3, 2016
Member

Huh, I'm confused why this test result changed. The test doesn't use responseURL at all.

jdm reviewed Feb 3, 2016
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 3, 2016

jdm self-assigned this Feb 3, 2016

highfive added S-tests-failed S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Feb 3, 2016

highfive added the S-needs-rebase label Feb 5, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 8, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 15, 2016

shinglyu force-pushed the shinglyu:responseurl branch from 27cbc47 to b513f50 Mar 21, 2016

highfive added the S-awaiting-review label Mar 21, 2016

jdm reviewed Mar 21, 2016
View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Mar 21, 2016

shinglyu force-pushed the shinglyu:responseurl branch from b513f50 to 268dc58 Mar 21, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 21, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 22, 2016

Bug 8830 - Implemented XMLHttpRequest.responseURL

Loading status checks…

f55b076

shinglyu force-pushed the shinglyu:responseurl branch from 268dc58 to f55b076 Mar 22, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 22, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 22, 2016

bors-servo merged commit f55b076 into servo:master Mar 22, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Implemented XMLHttpRequest.responseURL #9518

Implemented XMLHttpRequest.responseURL #9518

shinglyu commented Feb 3, 2016

KiChjang commented Feb 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 3, 2016

bors-servo commented Feb 3, 2016

bors-servo commented Feb 3, 2016

shinglyu commented Feb 4, 2016

shinglyu commented Feb 4, 2016

bors-servo commented Feb 5, 2016

jdm commented Feb 8, 2016

shinglyu commented Feb 15, 2016

jdm commented Feb 15, 2016

KiChjang commented Mar 21, 2016

shinglyu commented Mar 21, 2016

shinglyu commented Mar 21, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 21, 2016

jdm commented Mar 22, 2016

jdm commented Mar 22, 2016

bors-servo commented Mar 22, 2016

bors-servo commented Mar 22, 2016

bors-servo commented Mar 22, 2016

servo / servo

Join GitHub today

Implemented XMLHttpRequest.responseURL #9518

Implemented XMLHttpRequest.responseURL #9518

Conversation

shinglyu commented Feb 3, 2016

KiChjang commented Feb 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 3, 2016

bors-servo commented Feb 3, 2016

bors-servo commented Feb 3, 2016

shinglyu commented Feb 4, 2016

shinglyu commented Feb 4, 2016

bors-servo commented Feb 5, 2016

jdm commented Feb 8, 2016

shinglyu commented Feb 15, 2016

jdm commented Feb 15, 2016

KiChjang commented Mar 21, 2016

shinglyu commented Mar 21, 2016

shinglyu commented Mar 21, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 21, 2016

jdm commented Mar 22, 2016

jdm commented Mar 22, 2016

bors-servo commented Mar 22, 2016

bors-servo commented Mar 22, 2016

bors-servo commented Mar 22, 2016