Introduce ContainerNode::insertNodeVector() to share code in insertBefore(), appendChild(), and rep…

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 d948b388f951a9e755aad04c8b9da44f09f8c28c..6f5a96a07b4810812a19592b627142512a5ad567 100644
--- a/third_party/WebKit/Source/core/dom/ContainerNode.cpp
+++ b/third_party/WebKit/Source/core/dom/ContainerNode.cpp
@@ -150,6 +150,25 @@ bool ContainerNode::checkAcceptChildGuaranteedNodeTypes(const Node& newChild, co
     return true;
 }
 
+void ContainerNode::insertNodeVector(const NodeVector& targets, Node* next, bool (ContainerNode::*mutator)(Node& child, Node* next))

[tkent, 2016/09/07 04:39:23]

Functor is a smarter solution.  But I feel functor is overkill in this case.

[hayato, 2016/09/07 04:56:50]

I see. However, I'm afraid that `mutator` can not be inlined, though Functor
with template can be inlined, I think.

I am not sure which is better in this case. It looks a cost/benefits issue.
Could we measure the performance difference between them before landing the CL?

[tkent, 2016/09/07 07:28:02]

I observed no performance differences with dromaeo.domcoremodify and macOS
official build.  However, we might have regressions on other configurations. 
So, I have changed my mind.  Patch Set 2 applies functors.

+{
+    InspectorInstrumentation::willInsertDOMNode(this);
+    for (const auto& targetNode : targets) {
+        DCHECK(targetNode);
+        {
+            EventDispatchForbiddenScope assertNoEventDispatch;
+            ScriptForbiddenScope forbidScript;
+
+            if (!(this->*mutator)(*targetNode, next))
+                break;
+        }
+
+        updateTreeAfterInsertion(*targetNode);
+    }
+
+    dispatchSubtreeModifiedEvent();
+}
+
 Node* ContainerNode::insertBefore(Node* newChild, Node* refChild, ExceptionState& exceptionState)
 {
     // insertBefore(node, 0) is equivalent to appendChild(node)
@@ -191,36 +210,26 @@ Node* ContainerNode::insertBefore(Node* newChild, Node* refChild, ExceptionState
         return newChild;
     }
 
-    InspectorInstrumentation::willInsertDOMNode(this);
-
     ChildListMutationScope mutation(*this);
-    for (const auto& targetNode : targets) {
-        DCHECK(targetNode);
-        Node& child = *targetNode;
-
-        // 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())
-            break;
-
-        {
-            EventDispatchForbiddenScope assertNoEventDispatch;
-            ScriptForbiddenScope forbidScript;
-
-            treeScope().adoptIfNeeded(child);
-            insertBeforeCommon(*next, child);
-        }
-
-        updateTreeAfterInsertion(child);
-    }
+    insertNodeVector(targets, next, &ContainerNode::insertBeforeCommonWithAdoption);
+    return newChild;
+}
 
-    dispatchSubtreeModifiedEvent();
+bool ContainerNode::insertBeforeCommonWithAdoption(Node& newChild, Node* next)
+{
+    DCHECK(next);
+    // 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)
+        return false;
+    if (newChild.parentNode())
+        return false;
 
-    return newChild;
+    treeScope().adoptIfNeeded(newChild);
+    insertBeforeCommon(*next, newChild);
+    return true;
 }
 
 void ContainerNode::insertBeforeCommon(Node& nextChild, Node& newChild)
@@ -249,6 +258,18 @@ void ContainerNode::insertBeforeCommon(Node& nextChild, Node& newChild)
     newChild.setNextSibling(&nextChild);
 }
 
+bool ContainerNode::appendChildCommonWithAdoption(Node& child, Node*)
+{
+    // If the child has a parent again, just stop what we're doing, because that
+    // means someone is doing something with DOM mutation -- can't re-parent a
+    // child that already has a parent.
+    if (child.parentNode())
+        return false;
+    treeScope().adoptIfNeeded(child);
+    appendChildCommon(child);
+    return true;
+}
+
 void ContainerNode::appendChildCommon(Node& child)
 {
     child.setParentOrShadowHostNode(this);
@@ -366,37 +387,10 @@ Node* ContainerNode::replaceChild(Node* newChild, Node* oldChild, ExceptionState
         return child;
     }
 
-    InspectorInstrumentation::willInsertDOMNode(this);
-
-    // Add the new child(ren).
-    for (const auto& targetNode : targets) {
-        DCHECK(targetNode);
-        Node& child = *targetNode;
-
-        // 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 && next->parentNode() != this)
-            break;
-        if (child.parentNode())
-            break;
-
-        treeScope().adoptIfNeeded(child);
-
-        // Add child before "next".
-        {
-            EventDispatchForbiddenScope assertNoEventDispatch;
-            if (next)
-                insertBeforeCommon(*next, child);
-            else
-                appendChildCommon(child);
-        }
-
-        updateTreeAfterInsertion(child);
-    }
-
-    dispatchSubtreeModifiedEvent();
+    if (next)
+        insertNodeVector(targets, next, &ContainerNode::insertBeforeCommonWithAdoption);
+    else
+        insertNodeVector(targets, nullptr, &ContainerNode::appendChildCommonWithAdoption);
     return child;
 }
 
@@ -625,32 +619,8 @@ Node* ContainerNode::appendChild(Node* newChild, ExceptionState& exceptionState)
         return newChild;
     }
 
-    InspectorInstrumentation::willInsertDOMNode(this);
-
-    // Now actually add the child(ren).
     ChildListMutationScope mutation(*this);
-    for (const auto& targetNode : targets) {
-        DCHECK(targetNode);
-        Node& child = *targetNode;
-
-        // If the child has a parent again, just stop what we're doing, because
-        // that means someone is doing something with DOM mutation -- can't re-parent
-        // a child that already has a parent.
-        if (child.parentNode())
-            break;
-
-        {
-            EventDispatchForbiddenScope assertNoEventDispatch;
-            ScriptForbiddenScope forbidScript;
-
-            treeScope().adoptIfNeeded(child);
-            appendChildCommon(child);
-        }
-
-        updateTreeAfterInsertion(child);
-    }
-
-    dispatchSubtreeModifiedEvent();
+    insertNodeVector(targets, nullptr, &ContainerNode::appendChildCommonWithAdoption);
     return newChild;
 }