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.

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

Implement HTMLHyperlinkElementUtils for HTMLAnchorElement #9887

Merged
merged 1 commit into from Mar 25, 2016

Conversation

schuster
Copy link
Contributor

@schuster schuster commented Mar 5, 2016

Fixes #7857

Origin is omitted since it's still not available in rust-url, but since the previous PR also left it out, I'm assuming that's okay.

Please let me know if there are any style issues. There might be more concise ways to do the pattern matching that I don't know about, I guessed at the indentation style in one or two places.

Review on Reviewable

@highfive
Copy link

highfive commented Mar 5, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 5, 2016
@wafflespeanut wafflespeanut removed their assignment Mar 6, 2016
@frewsxcv
Copy link
Contributor

frewsxcv commented Mar 6, 2016

components/script/dom/htmlanchorelement.rs, line 164 [r1] (raw file):
Nit: if let Some(url) = self.url.borrow_mut().as_mut()


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Contributor

frewsxcv commented Mar 6, 2016

components/script/dom/htmlanchorelement.rs, line 201 [r1] (raw file):
Nit: if let Some(url) = self.url.borrow_mut().as_mut()


Comments from the review on Reviewable.io

@schuster
Copy link
Contributor Author

schuster commented Mar 9, 2016

Fixed nits and the build issue, and removed a TODO comment that I forgot to remove.

@Manishearth
Copy link
Member

You also want to update the test expectations here. https://github.com/servo/servo/blob/master/tests/wpt/README.md

@jdm
Copy link
Member

jdm commented Mar 10, 2016

@bors-servo: try

@bors-servo
Copy link
Contributor

⌛ Trying commit 2b9c27a with merge 5af1886...

bors-servo pushed a commit that referenced this pull request Mar 10, 2016
Implement HTMLHyperlinkElementUtils for HTMLAnchorElement

Fixes #7857

Origin is omitted since it's still not available in rust-url, but since the previous PR also left it out, I'm assuming that's okay.

Please let me know if there are any style issues. There might be more concise ways to do the pattern matching that I don't know about, I guessed at the indentation style in one or two places.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9887)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - status-appveyor

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 10, 2016
@jdm
Copy link
Member

jdm commented Mar 10, 2016

Thank you for doing this work! My one concern is that the code that relies on UrlHelper to implement parts of the specification does not appear to follow the steps like "set url's path to the empty list" - UrlHelper disregards the result of the operation, and the code in UrlUtilsWrapper bails if parsing fails, for example.

-S-awaiting-review +S-needs-code-changes


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


components/script/dom/htmlanchorelement.rs, line 65 [r2] (raw file):
We will need to use a base URL from the element's Document here.


components/script/dom/htmlanchorelement.rs, line 74 [r2] (raw file):
This is missing the check for "blob" URLs.


components/script/dom/htmlanchorelement.rs, line 186 [r2] (raw file):
nit: indent this to match the statement below.


components/script/dom/htmlanchorelement.rs, line 264 [r2] (raw file):
nit: indent this to match the first argument, please.


components/script/dom/htmlanchorelement.rs, line 406 [r2] (raw file):
Add a TODO comment about supporting the document's character encoding as well, please.


Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 10, 2016
@jdm
Copy link
Member

jdm commented Mar 10, 2016

Take a look at http://build.servo.org/builders/mac-rel-wpt/builds/2107/steps/shell_2/logs/stdio for the tests that now pass.

@schuster
Copy link
Contributor Author

@jdm Re: UrlHelper, it looks like its other uses in the codebase (e.g. see URLMethods::SetPathname and LocationMethods::SetPathname) require similar steps in their specs. Should I just rewrite its methods to follow those specs and remove the uses of UrlUtilsWrapper? I don't know if other parts of the codebase rely on existing behavior, though.

@jdm
Copy link
Member

jdm commented Mar 11, 2016

We could probably modify UrlHelper to do the necessary truncation without needing to modify UrlUtilsWrapper.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 21, 2016
@schuster
Copy link
Contributor Author

I've updated the code to incorporate all of the above feedback. I think it's ready, but UrlHelper will likely need to be revisited if and when servo/rust-url#176 lands.

@jdm
Copy link
Member

jdm commented Mar 22, 2016

Great work! One last stylistic nit and this will be ready to merge!
-S-awaiting-review +S-needs-code-changes


Reviewed 22 of 22 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/script/dom/urlhelper.rs, line 105 [r3] (raw file):
We try to avoid using map for side effects. if let Some(path) = url.path_mut() { would be preferred.


Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 22, 2016
@bors-servo
Copy link
Contributor

💔 Test failed - gonk

@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 Mar 23, 2016
@larsbergstrom
Copy link
Contributor

@bors-servo retry

  • infra

@bors-servo
Copy link
Contributor

⌛ Testing commit d6e5e89 with merge f766a81...

bors-servo pushed a commit that referenced this pull request Mar 23, 2016
Implement HTMLHyperlinkElementUtils for HTMLAnchorElement

Fixes #7857

Origin is omitted since it's still not available in rust-url, but since the previous PR also left it out, I'm assuming that's okay.

Please let me know if there are any style issues. There might be more concise ways to do the pattern matching that I don't know about, I guessed at the indentation style in one or two places.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9887)
<!-- 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-tests-failed The changes caused existing tests to fail. labels Mar 23, 2016
@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 Mar 23, 2016
@nox
Copy link
Contributor

nox commented Mar 23, 2016

Tests with unexpected results:
  ▶ CRASH [expected TIMEOUT] /html/browsers/windows/noreferrer.html

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://f:b/c> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://f: /c> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://f:fifty-two/c> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://f:999999/c> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://f: 21 / b ? d # e > against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://[1::2]:3:4> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://2001::1> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://2001::1]> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://2001::1]:80> against <http://example.org/foo/bar>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://[www.google.com]/> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://user:pass@/> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://foo:-80/> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http:/:@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://user@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http:@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http:/@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <https:@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http:a:b@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http:/a:b@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://a:b@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http::@/www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http:@:www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http:/@:www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://@:www.example.com> against <about:blank>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://example example.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://Goo%20 goo%7C|.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://[]> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://[:]> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://GOO\xa0\u3000goo.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://\ufdd0zyx.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://%ef%b7%90zyx.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://\uff05\uff14\uff11.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://%ef%bc%85%ef%bc%94%ef%bc%91.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://\uff05\uff10\uff10.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://%ef%bc%85%ef%bc%90%ef%bc%90.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://%zz%66%a.com> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://%25> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://hello%00> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://192.168.0.257> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://%3g%78%63%30%2e%30%32%35%30%2E.01> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://192.168.0.1 hello> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <http://[google.com]> against <http://other.com/>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <i> against <sc:sd>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <i> against <sc:sd/sd>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <../i> against <sc:sd>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <../i> against <sc:sd/sd>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: </i> against <sc:sd>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: </i> against <sc:sd/sd>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <?i> against <sc:sd>

  ▶ Unexpected subtest result in /url/a-element-xhtml.xhtml:
  └ PASS [expected FAIL] Parsing: <?i> against <sc:sd/sd>

@schuster
Copy link
Contributor Author

Hmm, I thought I updated all the test expectations, but must have somehow missed a few. I'll run it again and update.

The crash failure might be my fault - I'll investigate.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 25, 2016
@schuster
Copy link
Contributor Author

I updated the test expectations for a-element-xhtml.xhtml.

As far as the noreferrer.html test, from what I can tell it's crashing sometime after it gets the URL from the anchor element. So I think these changes just let that test get to the point where it can crash, rather than cause the crash themselves. I've marked that test expectation as CRASH, but please let me know if I should take some other action intead.

@jdm
Copy link
Member

jdm commented Mar 25, 2016

You're absolutely right. The test now navigates to another page that calls window.close(); this is supposed to happen in a new window, but in Servo it closes the whole browser window which looks like a crash to the test harness.

@jdm
Copy link
Member

jdm commented Mar 25, 2016

@bors-servo: r+
Thanks!

@bors-servo
Copy link
Contributor

📌 Commit c96c26b has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit c96c26b with merge 9a8d622...

@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 Mar 25, 2016
bors-servo pushed a commit that referenced this pull request Mar 25, 2016
Implement HTMLHyperlinkElementUtils for HTMLAnchorElement

Fixes #7857

Origin is omitted since it's still not available in rust-url, but since the previous PR also left it out, I'm assuming that's okay.

Please let me know if there are any style issues. There might be more concise ways to do the pattern matching that I don't know about, I guessed at the indentation style in one or two places.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9887)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit c96c26b into servo:master Mar 25, 2016
@@ -149,6 +153,7 @@ impl UrlHelper {
}

pub fn SetSearch(url: &mut Url, value: USVString) {
url.query = Some(String::new());
Copy link
Member

Choose a reason for hiding this comment

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

I’m rebasing #9840 which basically rewrites this file. Why was this change (and others in this file) necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implements step 5.2 from the spec for the search setter: https://html.spec.whatwg.org/multipage/semantics.html#dom-hyperlink-search . From what I've seen, all the uses of UrlHelper have a similar spec, so it made sense to put this here rather than, e.g., in htmlanchorelement.rs. Same for the other changes.

@jdm and I discussed this a little earlier in this PR thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet