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

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2013 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 132 matching lines @@
         exceptionState.throwDOMException(HierarchyRequestError, "The new child element contains the parent.");
         return false;
     }
     if (!isChildTypeAllowed(newChild)) {
         exceptionState.throwDOMException(HierarchyRequestError, "Nodes of type '" + newChild.nodeName() + "' may not be inserted inside nodes of type '" + nodeName() + "'.");
         return false;
     }
     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)
     if (!refChild) {
         return appendChild(newChild, exceptionState);
     }
 
     // Make sure adding the new child is OK.
     if (!checkAcceptChild(newChild, 0, exceptionState)) {
         if (exceptionState.hadException())
@@ skipping 21 matching lines @@
     if (targets.isEmpty())
         return newChild;
 
     // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.
     if (!checkAcceptChildGuaranteedNodeTypes(*newChild, nullptr, exceptionState)) {
         if (exceptionState.hadException())
             return nullptr;
         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);
-    }
-
-    dispatchSubtreeModifiedEvent();
-
+    insertNodeVector(targets, next, &ContainerNode::insertBeforeCommonWithAdoption);
     return newChild;
 }
 
+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;
+
+    treeScope().adoptIfNeeded(newChild);
+    insertBeforeCommon(*next, newChild);
+    return true;
+}
+
 void ContainerNode::insertBeforeCommon(Node& nextChild, Node& newChild)
 {
     EventDispatchForbiddenScope assertNoEventDispatch;
     ScriptForbiddenScope forbidScript;
 
     DCHECK(!newChild.parentNode()); // Use insertBefore if you need to handle reparenting (and want DOM mutation events).
     DCHECK(!newChild.nextSibling());
     DCHECK(!newChild.previousSibling());
     DCHECK(!newChild.isShadowRoot());
 
     Node* prev = nextChild.previousSibling();
     DCHECK_NE(m_lastChild, prev);
     nextChild.setPreviousSibling(&newChild);
     if (prev) {
         DCHECK_NE(firstChild(), nextChild);
         DCHECK_EQ(prev->nextSibling(), nextChild);
         prev->setNextSibling(&newChild);
     } else {
         DCHECK(firstChild() == nextChild);
         m_firstChild = &newChild;
     }
     newChild.setParentOrShadowHostNode(this);
     newChild.setPreviousSibling(prev);
     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);
 
     if (m_lastChild) {
         child.setPreviousSibling(m_lastChild);
         m_lastChild->setNextSibling(&child);
     } else {
         setFirstChild(&child);
     }
@@ skipping 97 matching lines @@
     if (exceptionState.hadException())
         return nullptr;
 
     // Does this yet another check because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
     if (!checkAcceptChild(newChild, child, exceptionState)) {
         if (exceptionState.hadException())
             return nullptr;
         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;
 }
 
 void ContainerNode::willRemoveChild(Node& child)
 {
     DCHECK_EQ(child.parentNode(), this);
     ChildListMutationScope(*this).willRemoveChild(child);
     child.notifyMutationObserversNodeWillDetach();
     dispatchChildRemovalEvents(child);
     ChildFrameDisconnector(child).disconnect();
@@ skipping 208 matching lines @@
     if (targets.isEmpty())
         return newChild;
 
     // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.
     if (!checkAcceptChildGuaranteedNodeTypes(*newChild, nullptr, exceptionState)) {
         if (exceptionState.hadException())
             return nullptr;
         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;
 }
 
 void ContainerNode::parserAppendChild(Node* newChild)
 {
     DCHECK(newChild);
     DCHECK(!newChild->isDocumentFragment());
     DCHECK(!isHTMLTemplateElement(this));
 
     if (!checkParserAcceptChild(*newChild))
@@ skipping 734 matching lines @@
         return true;
 
     if (node->isElementNode() && toElement(node)->shadow())
         return true;
 
     return false;
 }
 #endif
 
 } // namespace blink