[webkit-reviews] review granted: [Bug 103372] checkAcceptChild() needs fewer virtual calls : [Attachment 176445] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Hajime Morrita
<morrita at google.com>'s request for review:
Bug 103372: checkAcceptChild() needs fewer virtual calls
https://bugs.webkit.org/show_bug.cgi?id=103372

Attachment 176445: Patch
https://bugs.webkit.org/attachment.cgi?id=176445&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176445&action=review


> Source/WebCore/ChangeLog:15
> +	   - Added a fast path in checkAcceptChild(), where we canassume that
the parent is element

typo: canassume

> Source/WebCore/ChangeLog:17
> +	     and just needs a circle prevention through Node::contains(). This
is faster than current generic version.

nit: s/circle/cycle. circle is correct, but it's just a less common term for
this.

> Source/WebCore/dom/ContainerNode.cpp:153
> +    if ((newChild->isElementNode() || newChild->isTextNode()) &&
newParent->isElementNode()) {

Maybe add a comment above this line that this is just a fast-path for the
common case?

> Source/WebCore/dom/ContainerNode.cpp:157
> +	   ASSERT(!oldChild || !newParent->isDocumentNode() ||
static_cast<Document*>(newParent)->canReplaceChild(newChild, oldChild));

I think the last two clauses here are not especially useful since the
if-statement clearly doesn't go down this codepath if newParent is a Document.

> Source/WebCore/dom/ContainerNode.cpp:174
> +    } else {
> +	   if (!isChildTypeAllowed(newParent, newChild))

Nit: you could make this an "} else if {"

> Source/WebCore/dom/ContainerNode.cpp:371
> -    checkReplaceChild(newChild.get(), oldChild, ec);
> -    if (ec)
> +    if (!checkReplaceChild(this, newChild.get(), oldChild, ec))

This would be a separate patch, but I'm noting it while I see it. We only need
to do these checks again if we dispatched any events. We should just tracked
whether any events were dispatched and only do these checks if that was the
case (kind of like ChildNodesLazySnapshot).

Also, maybe I'm reading the code wrong, but I think there's a bug in
insertBefore/appendChild for the cases where we dispatch a sync event (e.g.
blur) during the removal in collectChildrenAndRemoveFromOldParent. We don't do
the checks again to see that we can still insert the node.