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

Make update_href accept a &Url argument instead of DOMString #11280

Open
jdm opened this issue May 19, 2016 · 5 comments
Open

Make update_href accept a &Url argument instead of DOMString #11280

jdm opened this issue May 19, 2016 · 5 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented May 19, 2016

This is a followup to the changes in #11264. This will allow removing some duplication in all of the callers of update_href in htmlanchorelement.rs. We can call it from inside the match, rather that storing an intermediary value before calling it.

Code: components/script/dom/htmlanchorelement.rs
Tests: ./mach test-wpt tests/wpt/web-platform-tests/html/semantics/text-level-semantics/the-a-element/ (should continue to pass)

@KiChjang
Copy link
Member

@KiChjang KiChjang commented May 19, 2016

I suspected that the crash in #11261 was precisely because update_href was called from within the match, since self.url is borrowed mutably in the match expression, and subsequently borrowed again in update_href.

@metajack
Copy link
Contributor

@metajack metajack commented May 22, 2016

@jdm #11261 made the exact opposite change on purpose. Did we not want to make that change? Is there some other way to address the issue @KiChjang was concerned about?

@jdm
Copy link
Member Author

@jdm jdm commented May 24, 2016

#11261 never established a cause and effect, since update_href clearly contains no borrow ofself.url.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented May 24, 2016

But the logs in #11261 clearly does state that there's a call to .borrow() in update_href, and @paulrouget reported no problems after applying #11264 locally.

@jdm
Copy link
Member Author

@jdm jdm commented May 24, 2016

Removing the easy label until we resolve #11281.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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