Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update URL-related interfaces and their tests up to spec #8008

Merged
merged 1 commit into from Oct 19, 2015
Merged

Update URL-related interfaces and their tests up to spec #8008

merged 1 commit into from Oct 19, 2015

Conversation

nox
Copy link
Contributor

@nox nox commented Oct 13, 2015

The URL spec recently changed and the variour "mixins" interfaces are gone,
this commit updates our code and WPT accordingly.

The new expected failures related to HTMLAnchorElement and HTMLAreaElement's
attributes are due to their moving to the HTMLHyperLinkElementUtils interface,
which is not anymore in a separate <script class=untested> element.

Review on Reviewable

@nox nox added the A-content/dom Interacting with the DOM from web content label Oct 13, 2015
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 13, 2015
This was referenced Oct 14, 2015
Closed
Closed
dzbarsky
dzbarsky reviewed Oct 14, 2015
View reviewed changes
components/script/dom/urlhelper.rs
pub fn SetProtocol(url: &mut Url, value: USVString) {
let mut wrapper = UrlUtilsWrapper { url: url, parser: &UrlParser::new() };
let _ = wrapper.set_scheme(&value.0);
}

// https://html.spec.whatwg.org/multipage/#same-origin
// g
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@nox
Copy link
Contributor Author

nox commented Oct 14, 2015

@dzbarsky Fat-fingered this, good catch.

@jdm jdm self-assigned this Oct 14, 2015
@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 19, 2015
@nox
Update URL-related interfaces and their tests up to spec
57c423a 
The URL spec recently changed and the variour "mixins" interfaces are gone,
this commit updates our code and WPT accordingly.

The new expected failures related to HTMLAnchorElement and HTMLAreaElement's
attributes are due to their moving to the HTMLHyperLinkElementUtils interface,
which is not anymore in a separate <script class=untested> element.
@nox nox removed the S-needs-rebase There are merge conflict errors. label Oct 19, 2015
@jdm
Copy link
Member

jdm commented Oct 19, 2015

@bors-servo: r+
Please note the requests to file followup issues!

Reviewed 19 of 19 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

components/script/dom/location.rs, line 73 [r1] (raw file):
According to the spec we should be invoking Location-object-setter in all of these setter methods. Presumably that should have been the case before, so I'm ok with filing it as a followup.

components/script/dom/location.rs, line 102 [r1] (raw file):
Please file a followup issue to make this and SetProtocol throw per the spec.

components/script/dom/webidls/HTMLAreaElement.webidl, line 16 [r1] (raw file):
What's the purpose of this comment?

components/script/dom/webidls/HTMLHyperlinkElementUtils.webidl, line 8 [r1] (raw file):
Why not at least uncomment the interface and annotations?

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

📌 Commit 57c423a has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 57c423a with merge f73cd40...

bors-servo pushed a commit that referenced this pull request Oct 19, 2015
Auto merge of #8008 - nox:url, r=jdm
f73cd40 
Update URL-related interfaces and their tests up to spec

The URL spec recently changed and the variour "mixins" interfaces are gone,
this commit updates our code and WPT accordingly.

The new expected failures related to HTMLAnchorElement and HTMLAreaElement's
attributes are due to their moving to the HTMLHyperLinkElementUtils interface,
which is not anymore in a separate `<script class=untested>` element.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8008)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 19, 2015
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 19, 2015
@jdm
Copy link
Member

jdm commented Oct 19, 2015

Sigh. #7625 strikes again.

@nox
Copy link
Contributor Author

nox commented Oct 19, 2015

@bors-servo retry

Seems like it's ok again?

@bors-servo
Copy link
Contributor

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

@nox nox removed the S-tests-failed The changes caused existing tests to fail. label Oct 19, 2015
@nox
Copy link
Contributor Author

nox commented Oct 19, 2015

Never mind, misread the bot in the IRC channel…

@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 57c423a into servo:master Oct 19, 2015
@nox nox deleted the url branch October 19, 2015 23:52
@jdm jdm mentioned this pull request Oct 26, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
A-content/dom Interacting with the DOM from web content
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nox @bors-servo @jdm @dzbarsky @highfive