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 Node.normalize() normalizes all descendants #2219

Closed
wants to merge 18 commits into from

Conversation

@hmac
Copy link
Contributor

hmac commented Apr 24, 2014

Fixes #2170 by recursively calling normalize() on all children that aren't text nodes.

At the time of writing the master branched passed 2/3 of the Node-normalize wpt tests. This branch passes 3/3.

I wrestled for a while with the compiler over pointers and lost, resorting to clone(). I imagine there's a way of doing it that avoids that but I couldn't work it out.

@highfive
Copy link

highfive commented Apr 24, 2014

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Apr 24, 2014

Critic review: https://critic.hoppipolla.co.uk/r/1371

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member

jdm commented Apr 24, 2014

If you squash these, I'll r+. What you did is what's necessary right; it will be nicer when #2101 is merged and no argument is necessary.

lpy and others added 18 commits Apr 20, 2014
Also implement it for DocumentFragment
This fixes two `RefCell<T> is already borrowed` failures when reloading an
existing pipeline, both caused by functions trying to modify `Pipeline::url`
or `ScriptTask::url` while a reference to a previous borrow is still in scope.
…work-related reasons.

Under the hood, this requires treating the I Tried pipeline as a new load instead of a replacement, since the failure-handling code interacts poorly with the rest of the replacement code when we get a series of staggered failures over time from the various pipeline components.
@hmac
Copy link
Contributor Author

hmac commented Apr 24, 2014

damn, that's not what I wanted to do. I'll scrap this and try again.

@hmac hmac closed this Apr 24, 2014
@hmac hmac deleted the hmac:normalize branch Apr 24, 2014
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.

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