[webkit-reviews] review granted: [Bug 58695] Creating copy of ContainerNode's when inserting or removing is inefficient : [Attachment 90652] Updated patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has granted 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 90652: Updated patch
https://bugs.webkit.org/attachment.cgi?id=90652&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=90652&action=review

r=me but please consider the style comments and ideally explain why the guards
here are safe and sufficient.

> Source/WebCore/dom/ContainerNode.cpp:372
> +    RefPtr<Node> protect = this;

I suggest the following as the more common WebKit idiom:

RefPtr<Node> protect(this);

> Source/WebCore/dom/ContainerNode.cpp:378
> +    for (RefPtr<Node> child = firstChild(); child; child =
child->nextSibling()) {
> +	   if (child->parentNode() != this) // Check for child being removed
from subtree while removing.
> +	       break;
> +	   child->willRemove();
> +    }

Won't this possibly change behavior for nodes inserted into the subtree while
removing? Is that ok? Doesn't seem like the old code is particularly sensible
either). Also, is it really correct that we won't send willRemove() for any
other children if one has been removed? That seems like it could be
problematic.

> Source/WebCore/dom/ContainerNode.cpp:755
> +    // NOTE: Since insertedIntoDocument can cause arbitrary changes to the 
> +    // document, we need to guard our data with RefPtrs and guard each
operation
> +    // with checks for mutation.
> +    RefPtr<Node> protect = this;

Again I suggest

RefPtr<Node> protect(this);

Also, it might be nice to abbreviate the comment a little. The self-protecting
RefPtr is a common WebKit idiom, it is not necessary to explain the idiom, just
briefly note why self-destruction is possible. The guard after each operation
is also already explained below.

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

Is it really right that we won't call insertedIntoDocument() on any subsequent
children if even one has been removed?