Implement attribute getter of Location #5591 #6135

shinglyu · 2015-05-19T16:14:31Z

hoppipolla-critic-bot · 2015-05-19T16:14:37Z

Critic review: https://critic.hoppipolla.co.uk/r/5051

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

larsbergstrom · 2015-05-19T21:34:48Z

r? @Ms2ger @jdm @Manishearth

Manishearth · 2015-05-20T03:21:19Z

components/script/dom/urlhelper.rs

+        // FIXME: Url null check is skipped for now
+        let host = match url.host() {
+            None => "".to_owned(),
+            Some(host) => format!("{}", host)


nit: we format host twice here, perhaps we shouldn't (instead do all formatting in the match below)

Manishearth · 2015-05-20T03:27:01Z

Could you file a followup about making these attributes writeable, and about fixing the nullable-URL thing? (the current implementation using Window is wrong; I think).

Or just fix them here :)

I think the url should be an Option<Url> or something; not referencing an existing object, and nullable. The constructors should set it from the window or elsewhere as per the steps in the spec.

(I haven't read through the spec, just making guesses from the portions I saw, so I could be wrong here)

shinglyu · 2015-05-20T13:02:34Z

Per @Ms2ger's suggestion, I'll add a test to data url.

@Manishearth I think I'll open a followup for writable and nullable url. I don't want to block other people for too long. :)

shinglyu · 2015-05-20T14:05:34Z

@Ms2ger : It turns out that there are two problems the prevent the data url test for location getters:

#5500 prevents me from using iframe.contentWindow.location, the contentWindow will be null if I don't use a timeout
Most of the location getters will return "", but the null check is not enabled. So I'll enable them (and add the test) in the followup bug.

shinglyu · 2015-05-20T14:16:29Z

Review status: 0 of 11 files reviewed, 1 unresolved discussion, all commit checks successful.

^{components/script/dom/urlhelper.rs, line 65 [r2] (raw file):}
Done. (I'm not sure about how to use Reviewable, should I mark this as done manually?)

Comments from the review on Reviewable.io

jdm · 2015-05-20T14:25:03Z

@shinglyu You can click the "acknowledge" button to mark issues as resolved in Reviewable.

Manishearth · 2015-05-23T18:52:51Z

Review status: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

components/script/dom/location.rs @ r1
components/script/dom/urlhelper.rs @ r3
components/script/dom/webidls/URLUtils.webidl @ r1
tests/wpt/metadata/html/browsers/history/the-location-interface/location_host.html.ini @ r2
tests/wpt/metadata/html/browsers/history/the-location-interface/location_hostname.html.ini @ r2
tests/wpt/metadata/html/browsers/history/the-location-interface/location_pathname.html.ini @ r2
tests/wpt/metadata/html/browsers/history/the-location-interface/location_port.html.ini @ r2
tests/wpt/metadata/html/browsers/history/the-location-interface/location_protocol.html.ini @ r2
tests/wpt/metadata/XMLHttpRequest/send-authentication-basic-cors.htm.ini @ r2
tests/wpt/metadata/XMLHttpRequest/send-authentication-basic-setrequestheader.htm.ini @ r2
tests/wpt/metadata/XMLHttpRequest/timeout-cors-async.htm.ini @ r2

Comments from the review on Reviewable.io

Manishearth · 2015-05-23T18:53:08Z

@bors-servo: r+

bors-servo · 2015-05-23T18:53:09Z

📌 Commit dc63efb has been approved by Manishearth

bors-servo · 2015-05-23T18:53:10Z

⌛ Testing commit dc63efb with merge 9511425...


        Auto merge of #6135 - shinglyu:5597, r=Manishearth

@yodalee

Reopening @yodalee 's #5597  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6135)

bors-servo · 2015-05-23T18:58:04Z

💔 Test failed - mac1

Manishearth · 2015-05-23T18:59:01Z


/Users/servo/buildbot/slave/mac1/build/components/script/dom/urlhelper.rs:65:13: 65:17 error: expected one of `,`, `.`, `}`, or an operator, found `Some`
/Users/servo/buildbot/slave/mac1/build/components/script/dom/urlhelper.rs:65             Some(host) => host

shinglyu · 2015-05-24T12:22:13Z

Oops, let me fix that right away

jdm · 2015-06-01T21:57:38Z

@shinglyu Could you squash the compile error into the original commit?

bors-servo · 2015-06-02T12:47:25Z

⌛ Testing commit 3723684 with merge 3ede47e...


        Auto merge of #6135 - shinglyu:5597, r=jdm

@yodalee

Reopening @yodalee 's #5597  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6135)

bors-servo · 2015-06-02T13:17:01Z

💔 Test failed - mac1

shinglyu · 2015-06-02T13:19:48Z

I remember I dealt with those tests before, let me check. Sorry!

jdm · 2015-06-02T13:19:54Z

/XMLHttpRequest/send-after-setting-document-domain.htm
------------------------------------------------------
FAIL loading documents from original origin after setting document.domain
OK expected TIMEOUT [Parent]
/XMLHttpRequest/timeout-cors-async.htm
--------------------------------------
FAIL XMLHttpRequest: timeout event and cross-origin request
/workers/WorkerLocation_host.htm
--------------------------------
FAIL  WorkerLocation URL decomposition IDL attribute: host 
/workers/WorkerLocation_hostname.htm
------------------------------------
FAIL  WorkerLocation URL decomposition IDL attribute: hostname 
/workers/WorkerLocation_port.htm
--------------------------------
FAIL  WorkerLocation URL decomposition IDL attribute: port 
/workers/WorkerLocation_protocol.htm
------------------------------------
FAIL  WorkerLocation URL decomposition IDL attribute: protocol

shinglyu · 2015-06-02T13:55:47Z

I disabled those tests locally, but didn't commit the .inis. Let me check if they should be disabled or not.

jdm · 2015-06-02T15:02:21Z

Disabling (ie. not running) is the wrong choice here. Those are valid results; the tests now check more things because they were previously stopping silently on properties of location that didn't exist.

snf · 2015-06-03T16:54:20Z

These tests are failing because WorkerLocation implement URLUtilsReadOnly which this patch does not include. These was passing before because both (URLUtils and URLUtilsReadOnly) returned undefined. I think the right choice would be to make the tests fail and wait for another patch to implement URLUtilsReadOnly.
I was working in this but I noticed this PR so I will work it from here (also have other patches for Worker)

shinglyu · 2015-06-04T09:10:24Z

Fernandez, thanks for the information, I'll disable them.

Sebastian N. Fernandez notifications@github.com 於 2015年6月4日週四上午12:54寫道：

These tests are failing because WorkerLocation implement URLUtilsReadOnly
which this patch does not include. These was passing before because both
(URLUtils and URLUtilsReadOnly) returned undefined. I think the right
choice would be to make the tests fail and wait for another patch to
implement URLUtilsReadOnly.
I was working in this but I noticed this PR so I will work it from here
(also have other patches for Worker)

—
Reply to this email directly or view it on GitHub
#6135 (comment).

snf · 2015-06-04T12:18:33Z

@shinglyu I think you have to change them to FAIL, not disable them.

shinglyu · 2015-06-05T01:29:35Z

@snf: sorry, I mean turn them to expected FAIL.

shinglyu · 2015-06-09T13:28:31Z

Seems like the WorkerLocation tests are failing due to the reason mentioned by @snf .
The XMLHttpRequest is failing because document.domain is undefined, which seems to be caused by #934

So can I change them to expected FAIL for now?

Ms2ger · 2015-06-12T12:24:45Z

Sure

nox · 2015-06-13T20:31:20Z

Err, didn't realise that my #6371 supersedes this.


        Implement attribute getter of Location #5591

shinglyu · 2015-06-14T13:07:24Z

@nox: sorry, I should have make this merge earlier.

vectorijk · 2015-06-14T17:22:34Z

@shinglyu I am a newbie and diving into how to implement the history api(forward and back). It may be helpful for me to add origin( https://url.spec.whatwg.org/#dom-urlutils-origin) into urlhelper.rs. Is that reasonable? Thanks~

shinglyu · 2015-06-15T01:47:58Z

@vectorijk : I am also quite new to this area, I think is reasonable, but you might need to consult someone who is more experienced then me.

bors-servo · 2015-06-21T16:59:54Z

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

shinglyu · 2015-07-01T12:39:20Z

#6371 implemented this. Should I close it?

jdm · 2015-07-01T14:32:37Z

I guess so!

highfive added the S-awaiting-review label May 19, 2015

Manishearth reviewed May 20, 2015
View changes

Manishearth added S-needs-code-changes and removed S-awaiting-review labels May 20, 2015

Manishearth self-assigned this May 20, 2015

shinglyu mentioned this pull request May 20, 2015

Issue5591 location getter #5597

Closed

highfive added S-awaiting-review and removed S-needs-code-changes labels May 20, 2015

This was referenced May 20, 2015

Make Location methods writable #6144

Closed

Null-check in location getters #6145

Closed

jdm added S-needs-code-changes and removed S-awaiting-review labels May 23, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels May 24, 2015

jdm added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Jun 2, 2015

jdm added S-tests-failed and removed S-awaiting-merge labels Jun 2, 2015

Implement attribute getter of Location #5591

Loading status checks…

7830f10

shinglyu force-pushed the shinglyu:5597 branch from 3723684 to 7830f10 Jun 14, 2015

highfive added S-awaiting-review and removed S-tests-failed labels Jun 14, 2015

jdm closed this Jul 1, 2015

servo / servo

Implement attribute getter of Location #5591 #6135

Implement attribute getter of Location #5591 #6135

shinglyu commented May 19, 2015

hoppipolla-critic-bot commented May 19, 2015

larsbergstrom commented May 19, 2015

This comment has been minimized.

Manishearth commented May 20, 2015

shinglyu commented May 20, 2015

shinglyu commented May 20, 2015

shinglyu commented May 20, 2015

jdm commented May 20, 2015

Manishearth commented May 23, 2015

Manishearth commented May 23, 2015

bors-servo commented May 23, 2015

bors-servo commented May 23, 2015

bors-servo commented May 23, 2015

Manishearth commented May 23, 2015

shinglyu commented May 24, 2015

jdm commented Jun 1, 2015

bors-servo commented Jun 2, 2015

bors-servo commented Jun 2, 2015

shinglyu commented Jun 2, 2015

jdm commented Jun 2, 2015

shinglyu commented Jun 2, 2015

jdm commented Jun 2, 2015

snf commented Jun 3, 2015

shinglyu commented Jun 4, 2015

snf commented Jun 4, 2015

shinglyu commented Jun 5, 2015

shinglyu commented Jun 9, 2015

Ms2ger commented Jun 12, 2015

nox commented Jun 13, 2015

shinglyu commented Jun 14, 2015

vectorijk commented Jun 14, 2015

shinglyu commented Jun 15, 2015

bors-servo commented Jun 21, 2015

shinglyu commented Jul 1, 2015

jdm commented Jul 1, 2015

servo / servo

Join GitHub today

Implement attribute getter of Location #5591 #6135

Implement attribute getter of Location #5591 #6135

Conversation

shinglyu commented May 19, 2015

hoppipolla-critic-bot commented May 19, 2015

larsbergstrom commented May 19, 2015

This comment has been minimized.

Manishearth May 20, 2015

Manishearth commented May 20, 2015

shinglyu commented May 20, 2015

shinglyu commented May 20, 2015

shinglyu commented May 20, 2015

jdm commented May 20, 2015

Manishearth commented May 23, 2015

Manishearth commented May 23, 2015

bors-servo commented May 23, 2015

bors-servo commented May 23, 2015

bors-servo commented May 23, 2015

Manishearth commented May 23, 2015

shinglyu commented May 24, 2015

jdm commented Jun 1, 2015

bors-servo commented Jun 2, 2015

bors-servo commented Jun 2, 2015

shinglyu commented Jun 2, 2015

jdm commented Jun 2, 2015

shinglyu commented Jun 2, 2015

jdm commented Jun 2, 2015

snf commented Jun 3, 2015

shinglyu commented Jun 4, 2015

snf commented Jun 4, 2015

shinglyu commented Jun 5, 2015

shinglyu commented Jun 9, 2015

Ms2ger commented Jun 12, 2015

nox commented Jun 13, 2015

shinglyu commented Jun 14, 2015

vectorijk commented Jun 14, 2015

shinglyu commented Jun 15, 2015

bors-servo commented Jun 21, 2015

shinglyu commented Jul 1, 2015

jdm commented Jul 1, 2015