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

Add comments linking to the steps of the specs in the checks on Location setters #25714

Closed
wants to merge 9 commits into from

add comments linking to steps of the specs

  • Loading branch information
UxioAndrade committed Feb 9, 2020
commit ad31fde3ef1203ba25047d4e9d2e134c1b764504
@@ -165,7 +165,7 @@ impl LocationMethods for Location {
// https://html.spec.whatwg.org/multipage/#dom-location-host
fn SetHost(&self, value: USVString) -> ErrorResult {
self.check_same_origin_domain()?;
// If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.
// Step 4: If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.
if !self.get_url().cannot_be_a_base() {
self.set_url_component(value, UrlHelper::SetHost);
}
@@ -187,7 +187,7 @@ impl LocationMethods for Location {
// https://html.spec.whatwg.org/multipage/#dom-location-hostname
fn SetHostname(&self, value: USVString) -> ErrorResult {
self.check_same_origin_domain()?;
// If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.
// Step 4: If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.
if !self.get_url().cannot_be_a_base() {
self.set_url_component(value, UrlHelper::SetHostname);
}
@@ -222,7 +222,7 @@ impl LocationMethods for Location {
// https://html.spec.whatwg.org/multipage/#dom-location-pathname
fn SetPathname(&self, value: USVString) -> ErrorResult {
self.check_same_origin_domain()?;
// If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.
// Step 3: If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.

This comment has been minimized.

Copy link
@gterzian

gterzian Feb 10, 2020

Member

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.

if !self.get_url().cannot_be_a_base() {
self.set_url_component(value, UrlHelper::SetPathname);
}
@@ -239,7 +239,7 @@ impl LocationMethods for Location {
fn SetPort(&self, value: USVString) -> ErrorResult {
self.check_same_origin_domain()?;
let url = self.get_url();
// If copyURL cannot have a username/password/port, then return.
// Step 4: If copyURL cannot have a username/password/port, then return.
// https://url.spec.whatwg.org/#cannot-have-a-username-password-port
if url.has_host() && !url.cannot_be_a_base() && url.scheme() != "file" {
self.set_url_component(value, UrlHelper::SetPort);
@@ -256,7 +256,7 @@ impl LocationMethods for Location {
// https://html.spec.whatwg.org/multipage/#dom-location-protocol
fn SetProtocol(&self, value: USVString) -> ErrorResult {
self.check_same_origin_domain()?;
// If copyURL's scheme is not an HTTP(S) scheme, then terminate these steps.
// Step 6: If copyURL's scheme is not an HTTP(S) scheme, then terminate these steps.
let scheme = value.split(':').next().unwrap();

This comment has been minimized.

Copy link
@paulrouget

paulrouget Feb 10, 2020

Contributor

Isn’t it dangerous to have unwrap() here?

This comment has been minimized.

Copy link
@gterzian

gterzian Feb 10, 2020

Member

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;

This comment has been minimized.

Copy link
@UxioAndrade

UxioAndrade Feb 10, 2020

Author

Would my latest commit fix that?

if scheme.eq_ignore_ascii_case("http") || scheme.eq_ignore_ascii_case("https") {
self.set_url_component(value, UrlHelper::SetProtocol);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.