[webkit-reviews] review granted: [Bug 82631] Delay post-insertion notifications until new DOM tree is complete : [Attachment 135658] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Adam Klein <adamk at chromium.org>'s
request for review:
Bug 82631: Delay post-insertion notifications until new DOM tree is complete
https://bugs.webkit.org/show_bug.cgi?id=82631

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

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


I'm uncomfortably excited about this change. :)

> Source/WebCore/dom/ContainerNode.cpp:169
>	   // Due to arbitrary code running in response to a DOM mutation event
it's
> -	   // possible that "next" is no longer a child of "this".
> -	   // It's also possible that "child" has been inserted elsewhere.
> -	   // In either of those cases, we'll just stop.
> -	   if (next->parentNode() != this)
> -	       break;
> -	   if (child->parentNode())
> +	   // possible that "child" has been inserted elsewhere.

Is the only case this happens now if you're appending a script element?

> Source/WebCore/dom/ContainerNode.cpp:175
>	   treeScope()->adoptIfNeeded(child);

Here and below, it might be worth adding a comment that adoptIfNeeded will
never fire sync JS events because we've already removed the child from it's
existing parent and so there are no DOM mutations involved here.

> Source/WebCore/dom/ContainerNode.cpp:1090
> +	   // FIXME: Attachment should be the first operation in this function,
but some code

Nit: s/this function/this loop/