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

Remove unnecessary calls to node_removed and change semantics of node_inserted #6229

Closed
mukilan opened this issue May 31, 2015 · 6 comments
Closed

Comments

@mukilan
Copy link
Contributor

@mukilan mukilan commented May 31, 2015

PR #5972 has changed the semantics of node_removed so that it now corresponds to the "removal steps" in the spec and doesn't perform any steps related to "mutation observers". Hence,
as explained here, the calls to node_removed from Node::ReplaceChild and Node::replace_all should be removed.

Additionally to keep things symmetric, node_inserted can also be called unconditionally from insert
and the calls from replace_all and ReplaceChild can be removed as well, so that node_inserted will correspond to the "insertion steps" in the spec.

@lambdabaa
Copy link

@lambdabaa lambdabaa commented Jun 1, 2015

I'd like to take!

@jdm
Copy link
Member

@jdm jdm commented Jun 2, 2015

Go for it!

@jdm jdm added the C-assigned label Jun 2, 2015
@jdm jdm removed the C-assigned label Jul 14, 2015
@nick-thompson
Copy link
Contributor

@nick-thompson nick-thompson commented Jul 22, 2015

@gaye are you working on this one? I'd like to take a stab at it if not.

@lambdabaa
Copy link

@lambdabaa lambdabaa commented Jul 22, 2015

@nick-thompson Be my guest! I got caught up in some other work.

@jdm jdm added the C-assigned label Jul 22, 2015
@nox
Copy link
Member

@nox nox commented Jul 23, 2015

I think I'm already covering this in #6660, you might want to work on something else. Sorry I didn't notice this ticket.

@nick-thompson
Copy link
Contributor

@nick-thompson nick-thompson commented Jul 23, 2015

Yep, looks like your commit covers all of the code I was about to change. Thanks for the heads up, I'll look for something else.

I'll leave this for a more senior contributor to verify and close out ;)

@jdm jdm closed this Jul 25, 2015
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.

None yet
5 participants
You can’t perform that action at this time.