Always notify subtree of removal in ContainerNode::removeChildren
Created attachment 128360 [details] Patch
Comment on attachment 128360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128360&action=review > Source/WebCore/dom/ContainerNode.cpp:605 > + removedChild->removedFromTree(true); Are you sure this won't cause any perf regressions?
Comment on attachment 128360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128360&action=review >> Source/WebCore/dom/ContainerNode.cpp:605 >> + removedChild->removedFromTree(true); > > Are you sure this won't cause any perf regressions? Presumably it _will_ cause a perf regression. But the old code is incorrect. Did you have an alternate implementation suggestion?
One thing that confuses me is that removedFromTree doesn't seem to be called when a non-container node that's in document tree gets removed. ContainerNode::removedFromDocument() calls removedFromTree(), but Node::removedFromDocument() does not. It seems that with this patch, removedFromTree() will sometimes be called for text nodes, but not always. What's the actual contract for this method? Of course, only ContainerNodes override it, so it doesn't matter in practice, but it would be good to have clarity here before making fixes.
(In reply to comment #4) > One thing that confuses me is that removedFromTree doesn't seem to be called when a non-container node that's in document tree gets removed. ContainerNode::removedFromDocument() calls removedFromTree(), but Node::removedFromDocument() does not. > > It seems that with this patch, removedFromTree() will sometimes be called for text nodes, but not always. What's the actual contract for this method? > > Of course, only ContainerNodes override it, so it doesn't matter in practice, but it would be good to have clarity here before making fixes. To give more context here, removedFromTree seems to be used exclusively for this form-association behavior. The only non-forwarding implementations I could find are in FormAssociatedElement and HTMLImageElement.
Any further thoughts from reviewers?
(In reply to comment #5) > To give more context here, removedFromTree seems to be used exclusively for this form-association behavior. The only non-forwarding implementations I could find are in FormAssociatedElement and HTMLImageElement. But that may change in the future. Can we add a comment where removedFromTree is called, or better yet rename removedFromTree to containerRemovedFromTree so that such a restraint is clear from the function name?
(In reply to comment #7) > (In reply to comment #5) > > To give more context here, removedFromTree seems to be used exclusively for this form-association behavior. The only non-forwarding implementations I could find are in FormAssociatedElement and HTMLImageElement. > > But that may change in the future. Can we add a comment where removedFromTree is called, or better yet rename removedFromTree to containerRemovedFromTree so that such a restraint is clear from the function name? I'm guessing you're referring to ap's comment (#6), not mine. Assuming that's the case: rather than simply renaming the method, it could be moved to ContainerNode. The only argument I can see for leaving it in Node is the section titled "Notification of document structure changes" in Node.h. But then, as you point out, this is misleading. While moving insertedIntoTree/removedFromTree, we could also move childrenChanged, which is particularly silly to be part of Node's interface. What do you think?
(In reply to comment #8) > I'm guessing you're referring to ap's comment (#6), not mine. Assuming that's the case: rather than simply renaming the method, it could be moved to ContainerNode. That'll be nice since it'll also avoid unnecessary virtual function calls. >The only argument I can see for leaving it in Node is the section titled "Notification of document structure changes" in Node.h. But then, as you point out, this is misleading. While moving insertedIntoTree/removedFromTree, we could also move childrenChanged, which is particularly silly to be part of Node's interface. What do you think? Yeah, I think that's a great idea. I still feel that we should rename the function to make the code that calls this function self-evident though.
(In reply to comment #9) > (In reply to comment #8) > > I'm guessing you're referring to ap's comment (#6), not mine. Assuming that's the case: rather than simply renaming the method, it could be moved to ContainerNode. > > That'll be nice since it'll also avoid unnecessary virtual function calls. > > >The only argument I can see for leaving it in Node is the section titled "Notification of document structure changes" in Node.h. But then, as you point out, this is misleading. While moving insertedIntoTree/removedFromTree, we could also move childrenChanged, which is particularly silly to be part of Node's interface. What do you think? > > Yeah, I think that's a great idea. I still feel that we should rename the function to make the code that calls this function self-evident though. Seems unnecessary, since most calls will be: toContainerNode(child)->removedFromTree() Let's see how it looks in the patch and we can consider renaming then.
Please note that there is extensive documentation for these methods in Node.h. It should definitely be fixed even if functions are not renamed.
(In reply to comment #10) > Seems unnecessary, since most calls will be: > toContainerNode(child)->removedFromTree() That's a good point.
The discussed refactoring is in https://bugs.webkit.org/show_bug.cgi?id=79697, please take a look.
Created attachment 129104 [details] Merged with trunk
Comment on attachment 129104 [details] Merged with trunk Clearing flags on attachment: 129104 Committed r109054: <http://trac.webkit.org/changeset/109054>
All reviewed patches have been landed. Closing bug.
Mass moving XML DOM bugs to the "DOM" Component.