Add comments linking to the steps of the specs in the checks on Location setters#25714
Add comments linking to the steps of the specs in the checks on Location setters#25714uxioandrade wants to merge 9 commits intoservo:masterfrom
Conversation
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon. |
|
Heads up! This PR modifies the following files:
|
|
Thanks! Could you please also squash into one commit? |
components/script/dom/location.rs
Outdated
| self.check_same_origin_domain()?; | ||
| self.set_url_component(value, UrlHelper::SetProtocol); | ||
| // Step 6: If copyURL's scheme is not an HTTP(S) scheme, then terminate these steps. | ||
| let scheme = value.split(':').next().unwrap(); |
There was a problem hiding this comment.
Isn’t it dangerous to have unwrap() here?
There was a problem hiding this comment.
Yes you're right, actually per Step 5 of the protocol setter spec, I think we should return a Error::Syntax here instead of unwrap, see https://html.spec.whatwg.org/multipage/history.html#dom-location-protocol
This would be using use crate::dom::bindings::error::Error;
There was a problem hiding this comment.
Would my latest commit fix that?
There was a problem hiding this comment.
Yes.
Please run ./mach fmt to fix https://community-tc.services.mozilla.com/tasks/VcHxks51RSywXhLH0W3dXg/runs/0/logs/https%3A%2F%2Fcommunity-tc.services.mozilla.com%2Fapi%2Fqueue%2Fv1%2Ftask%2FVcHxks51RSywXhLH0W3dXg%2Fruns%2F0%2Fartifacts%2Fpublic%2Flogs%2Flive.log
And please also squash...
|
Hi @gterzian should I squash all the commits or just the ones before I merged the master branch? |
|
I would say just one(and I think it will still automatically list @braddunbar as co-author, which would be a good thing). |
| fn SetPathname(&self, value: USVString) -> ErrorResult { | ||
| self.check_same_origin_domain()?; | ||
| self.set_url_component(value, UrlHelper::SetPathname); | ||
| // Step 3: If copyURL's cannot-be-a-base-URL flag is set, terminate these steps. |
There was a problem hiding this comment.
Actually, can we put this comment, and all others, above the Ok(()) branch, because the comments all refer to the steps take when the condition is not met.
| } | ||
| Ok(()) | ||
| } else { | ||
| Err(Error::Syntax) |
There was a problem hiding this comment.
Here we can put a comment for Step 5, throw a "SyntaxError" DOMException.
jdm
added
S-needs-code-changes
|
☔ The latest upstream changes (presumably #25931) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #25947) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #26039) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #26119) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@uxioandrade Are you planning on finishing up the PR? |
|
☔ The latest upstream changes (presumably #26222) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing due to inactivity. |
Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes
Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes
Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes
Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @uxioandrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes
8 participants
As stated in #23670 (comment), I added some comments to the checks on Location setters, so that it is easier to follow the steps in specs.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors