Bug 79316 – Always notify subtree of removal in ContainerNode::removeChildren

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Klein]

Always notify subtree of removal in ContainerNode::removeChildren

[Adam Klein]

Created attachment 128360 [details]
Patch

[Ryosuke Niwa]

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?

[Adam Klein]

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?

[Alexey Proskuryakov]

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.

[Adam Klein]

(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.

[Adam Klein]

Any further thoughts from reviewers?

[Ryosuke Niwa]

(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?

[Adam Klein]

(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?

[Ryosuke Niwa]

(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.

[Adam Klein]

(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.

[Alexey Proskuryakov]

Please note that there is extensive documentation for these methods in Node.h. It should definitely be fixed even if functions are not renamed.

[Ryosuke Niwa]

(In reply to comment #10)
> Seems unnecessary, since most calls will be:
> toContainerNode(child)->removedFromTree()

That's a good point.

[Adam Klein]

The discussed refactoring is in https://bugs.webkit.org/show_bug.cgi?id=79697, please take a look.

[Adam Klein]

Created attachment 129104 [details]
Merged with trunk

[WebKit Review Bot]

Comment on attachment 129104 [details]
Merged with trunk

Clearing flags on attachment: 129104

Committed r109054: <http://trac.webkit.org/changeset/109054>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.