Bug 79316

Summary: Always notify subtree of removal in ContainerNode::removeChildren
Product: WebKit Reporter: Adam Klein <adamk>
Component: DOMAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, darin, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79697    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Merged with trunk none

Description Adam Klein 2012-02-22 18:29:29 PST
Always notify subtree of removal in ContainerNode::removeChildren
Comment 1 Adam Klein 2012-02-22 18:30:49 PST
Created attachment 128360 [details]
Patch
Comment 2 Ryosuke Niwa 2012-02-22 19:07:52 PST
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 3 Adam Klein 2012-02-22 19:16:49 PST
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?
Comment 4 Alexey Proskuryakov 2012-02-23 10:45:57 PST
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.
Comment 5 Adam Klein 2012-02-23 10:49:32 PST
(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.
Comment 6 Adam Klein 2012-02-27 12:04:34 PST
Any further thoughts from reviewers?
Comment 7 Ryosuke Niwa 2012-02-27 12:08:19 PST
(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?
Comment 8 Adam Klein 2012-02-27 12:24:17 PST
(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?
Comment 9 Ryosuke Niwa 2012-02-27 12:57:24 PST
(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.
Comment 10 Adam Klein 2012-02-27 13:17:29 PST
(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.
Comment 11 Alexey Proskuryakov 2012-02-27 13:24:19 PST
Please note that there is extensive documentation for these methods in Node.h. It should definitely be fixed even if functions are not renamed.
Comment 12 Ryosuke Niwa 2012-02-27 13:26:00 PST
(In reply to comment #10)
> Seems unnecessary, since most calls will be:
> toContainerNode(child)->removedFromTree()

That's a good point.
Comment 13 Adam Klein 2012-02-27 14:02:19 PST
The discussed refactoring is in https://bugs.webkit.org/show_bug.cgi?id=79697, please take a look.
Comment 14 Adam Klein 2012-02-27 14:58:53 PST
Created attachment 129104 [details]
Merged with trunk
Comment 15 WebKit Review Bot 2012-02-27 17:51:51 PST
Comment on attachment 129104 [details]
Merged with trunk

Clearing flags on attachment: 129104

Committed r109054: <http://trac.webkit.org/changeset/109054>
Comment 16 WebKit Review Bot 2012-02-27 17:51:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Lucas Forschler 2019-02-06 09:02:56 PST
Mass moving XML DOM bugs to the "DOM" Component.