Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement HTMLHyperlinkElementUtils for HTMLAnchorElement #9887
Conversation
highfive
commented
Mar 5, 2016
|
|
|
components/script/dom/htmlanchorelement.rs, line 164 [r1] (raw file): Comments from the review on Reviewable.io |
|
components/script/dom/htmlanchorelement.rs, line 201 [r1] (raw file): Comments from the review on Reviewable.io |
schuster
force-pushed the
schuster:HTMLHyperlinkElementUtils
branch
from
4c0606d
to
2b9c27a
|
Fixed nits and the build issue, and removed a TODO comment that I forgot to remove. |
|
You also want to update the test expectations here. https://github.com/servo/servo/blob/master/tests/wpt/README.md |
|
@bors-servo: try |
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 -->
|
|
|
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. components/script/dom/htmlanchorelement.rs, line 65 [r2] (raw file): components/script/dom/htmlanchorelement.rs, line 74 [r2] (raw file): components/script/dom/htmlanchorelement.rs, line 186 [r2] (raw file): components/script/dom/htmlanchorelement.rs, line 264 [r2] (raw file): components/script/dom/htmlanchorelement.rs, line 406 [r2] (raw file): Comments from the review on Reviewable.io |
|
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. |
|
@jdm Re: |
|
We could probably modify UrlHelper to do the necessary truncation without needing to modify UrlUtilsWrapper. |
schuster
force-pushed the
schuster:HTMLHyperlinkElementUtils
branch
from
2b9c27a
to
a2ebf18
highfive
added
S-awaiting-review
and removed
S-tests-failed
S-needs-code-changes
labels
|
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. |
|
Great work! One last stylistic nit and this will be ready to merge! Reviewed 22 of 22 files at r3. components/script/dom/urlhelper.rs, line 105 [r3] (raw file): Comments from the review on Reviewable.io |
|
|
|
@bors-servo retry
|
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 -->
|
|
|
|
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. |
schuster
force-pushed the
schuster:HTMLHyperlinkElementUtils
branch
from
d6e5e89
to
1ff6a1f
schuster
force-pushed the
schuster:HTMLHyperlinkElementUtils
branch
from
1ff6a1f
to
c96c26b
|
I updated the test expectations for As far as the |
|
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. |
|
@bors-servo: r+ |
|
|
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 -->
|
|
| @@ -149,6 +153,7 @@ impl UrlHelper { | |||
| } | |||
|
|
|||
| pub fn SetSearch(url: &mut Url, value: USVString) { | |||
| url.query = Some(String::new()); | |||
This comment has been minimized.
This comment has been minimized.
SimonSapin
Mar 25, 2016
Member
I’m rebasing #9840 which basically rewrites this file. Why was this change (and others in this file) necessary?
This comment has been minimized.
This comment has been minimized.
schuster
Mar 25, 2016
Author
Contributor
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.
schuster commentedMar 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.