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

Ensure that a navigation to the same URL is aborted. Fixes #10952. #10954

Closed
wants to merge 1 commit into from

Conversation

@jdm
Copy link
Member

jdm commented May 1, 2016

This change is Reviewable

@highfive
Copy link

highfive commented May 1, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/script_thread.rs
@KiChjang
Copy link
Member

KiChjang commented May 1, 2016

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

r=me after that small nit.


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


components/script/script_thread.rs, line 1791 [r1] (raw file):
Can we move the above 3 lines into the if-body? It doesn't seem to be necessary to initialize and allocate resources for these variables if the condition turns out to be false.


Comments from Reviewable

@KiChjang
Copy link
Member

KiChjang commented May 1, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


components/script/script_thread.rs, line 1800 [r1] (raw file):
Hold on, the algorithm is only aborted when there is a fragment for nurl, so this should move 1 line up.


Comments from Reviewable

@emilio
Copy link
Member

emilio commented May 1, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


components/script/script_thread.rs, line 1800 [r1] (raw file):
Yes, I think Keith is right here, the problem is that setting location.hash to an empty string seems to follow this code path, which sets the url fragment to None instead of Some("").

This seems to be the expected behaviour given the spec section for location.hash:

If the given value is the empty string, set copyURL's fragment to null.

So not sure if the "navigate" algorithm spec should be updated to take into account null fragments, or if there's another bug in our code that should make us arrive with an url fragment (which doesn't seem to be the case, but I haven't looked too deeply at it).


Comments from Reviewable

@jdm
Copy link
Member Author

jdm commented May 3, 2016

I've filed whatwg/html#1177 about this.

@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2016

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

@emilio
Copy link
Member

emilio commented May 19, 2016

I think this should probably be rebased and landed while we wait for input in whatwg/html, there are quite a few pages affected.

What do you think @jdm? I can do the rebase if you're busy btw :)

@jdm
Copy link
Member Author

jdm commented May 19, 2016

I am inclined to agree, with a comment that links to the whatwg issue. If you could do the rebase, that would be swell!

@emilio
Copy link
Member

emilio commented May 21, 2016

Rebased in #11321

@emilio emilio closed this May 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.