[webkit-reviews] review denied: [Bug 58695] Creating copy of ContainerNode's when inserting or removing is inefficient : [Attachment 89857] Patch to re-implement Node handling without making copies

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 58695: Creating copy of ContainerNode's when inserting or removing is
inefficient
https://bugs.webkit.org/show_bug.cgi?id=58695

Attachment 89857: Patch to re-implement Node handling without making copies
https://bugs.webkit.org/attachment.cgi?id=89857&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89857&action=review

>> Source/WebCore/dom/ContainerNode.cpp:371
>> +	for (RefPtr<Node> child = firstChild(); child; child =
child->nextSibling())
>> +	    child->willRemove();
> 
> What will happen to the 3rd child, if calling willRemove on the 1st removes
the 2nd?  Won't it get left out in the cold? (aka, never get a willRemove()?)

What will happen if one of these nodes gets moved to another parent during this
operation?  It seems like we'll call willRemove on nodes that aren't actually
going to be removed.

> Source/WebCore/dom/ContainerNode.cpp:752
> +	   if (child->parentNode() != this) // Check for child being removed
from subtree while reparenting.
> +	       break;

What if child N gets moved to the first or last sibling position during this
operation?  It seems like we'll either call insertedIntoDocument too many or
too few times in that case, which could cause problems.