Change the timing of event dispatching and <script> execution in Node::appendChild, insertBefore,...

third_party/WebKit/Source/core/dom/ContainerNode.cpp

Index: third_party/WebKit/Source/core/dom/ContainerNode.cpp
diff --git a/third_party/WebKit/Source/core/dom/ContainerNode.cpp b/third_party/WebKit/Source/core/dom/ContainerNode.cpp
index 35e72222042f727a8808d2f804a360cde7f92763..b277ab739a8557379fe8f239435a01058b647837 100644
--- a/third_party/WebKit/Source/core/dom/ContainerNode.cpp
+++ b/third_party/WebKit/Source/core/dom/ContainerNode.cpp
@@ -154,16 +154,32 @@ template <typename Functor>
 void ContainerNode::insertNodeVector(const NodeVector& targets, Node* next, const Functor& mutator)
 {
     InspectorInstrumentation::willInsertDOMNode(this);
-    for (const auto& targetNode : targets) {
-        DCHECK(targetNode);
-        {
-            EventDispatchForbiddenScope assertNoEventDispatch;
-            ScriptForbiddenScope forbidScript;
-
-            if (!mutator(*this, *targetNode, next))
+    NodeVector postInsertionNotificationTargets;
+    {
+        EventDispatchForbiddenScope assertNoEventDispatch;
+        ScriptForbiddenScope forbidScript;
+        for (const auto& targetNode : targets) {
+            DCHECK(targetNode);
+            Node& child = *targetNode;
+            // TODO(tkent): mutator never returns false because scripts don't run in the loop.

[tkent, 2016/09/08 04:52:44]

Will remove unnecessary code in a follow-up CL.

+            if (!mutator(*this, child, next))
                 break;
+            ChildListMutationScope(*this).childAdded(child);
+            if (document().containsV1ShadowTree())
+                child.checkSlotChangeAfterInserted();
+            InspectorInstrumentation::didInsertDOMNode(&child);
+            notifyNodeInsertedInternal(child, postInsertionNotificationTargets);
         }
-        updateTreeAfterInsertion(*targetNode);
+    }
+    for (const auto& targetNode : targets)
+        childrenChanged(ChildrenChange::forInsertion(*targetNode, ChildrenChangeSourceAPI));
+    for (const auto& descendant : postInsertionNotificationTargets) {
+        if (descendant->isConnected())
+            descendant->didNotifySubtreeInsertionsToDocument();
+    }
+    for (const auto& targetNode : targets) {
+        if (targetNode->parentNode() == this)

[hayato, 2016/09/08 06:18:55]

Should we check this condition?
It looks the original code is always calling dispatchChildInsertionEvents.

[tkent, 2016/09/08 06:31:08]

The DOM specification is unclear for this and we may dispatch unconditionally,
but this check is compatible with Firefox and Edge.

With the original code, JavaScript execution for an earlier child by <script> or
DOM mutation event can't remove a later child from the parent because the later
child isn't appended to the parent yet.

[hayato, 2016/09/08 06:48:11]

We chatted offline. 

We have decided to make this align to Firefox and Edge, though the spec is
unclear.

+            dispatchChildInsertionEvents(*targetNode);
     }
     dispatchSubtreeModifiedEvent();
 }
@@ -1170,15 +1186,6 @@ static void dispatchChildRemovalEvents(Node& child)
     }
 }
 
-void ContainerNode::updateTreeAfterInsertion(Node& child)
-{
-    ChildListMutationScope(*this).childAdded(child);
-
-    notifyNodeInserted(child);
-
-    dispatchChildInsertionEvents(child);
-}
-
 bool ContainerNode::hasRestyleFlagInternal(DynamicRestyleFlags mask) const
 {
     return rareData()->hasRestyleFlag(mask);