Clean up child checks in ContainerNode.

sky/engine/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 55 matching lines @@
     removeDetachedChildrenInContainer(*this);
 }
 
 ContainerNode::~ContainerNode()
 {
     ASSERT(needsAttach());
     willBeDeletedFromDocument();
     removeDetachedChildren();
 }
 
-bool ContainerNode::isChildTypeAllowed(const Node& child) const
-{
-    if (!child.isDocumentFragment())
-        return childTypeAllowed(child.nodeType());
-
-    for (Node* node = toDocumentFragment(child).firstChild(); node; node = node->nextSibling()) {
-        if (!childTypeAllowed(node->nodeType()))
-            return false;
-    }
-    return true;
-}
-
 bool ContainerNode::containsConsideringHostElements(const Node& newChild) const
 {
     if (isInShadowTree() || document().isTemplateDocument())
         return newChild.containsIncludingHostElements(*this);
     return newChild.contains(this);
 }
 
-bool ContainerNode::checkAcceptChild(const Node* newChild, const Node* oldChild, ExceptionState& exceptionState) const
+void ContainerNode::checkAcceptChild(const Node* newChild, const Node* oldChild, ExceptionState& exceptionState) const
 {
     // Not mentioned in spec: throw NotFoundError if newChild is null
     if (!newChild) {
         exceptionState.throwDOMException(NotFoundError, "The new child element is null.");
-        return false;
+        return;
     }
 
-    // Use common case fast path if possible.
-    if ((newChild->isElementNode() || newChild->isTextNode()) && isElementNode()) {
-        ASSERT(isChildTypeAllowed(*newChild));
-        if (containsConsideringHostElements(*newChild)) {
-            exceptionState.throwDOMException(HierarchyRequestError, "The new child element contains the parent.");
-            return false;
-        }
-        return true;
+    if (newChild->isTreeScope()) {
+        exceptionState.throwDOMException(HierarchyRequestError, "Nodes of type '" + newChild->nodeName() + "' may not be inserted inside nodes of type '" + nodeName() + "'.");
+        return;
     }
 
     if (containsConsideringHostElements(*newChild)) {
         exceptionState.throwDOMException(HierarchyRequestError, "The new child element contains the parent.");
-        return false;
+        return;
     }
 
-    if (oldChild && isDocumentNode()) {
-        if (!toDocument(this)->canReplaceChild(*newChild, *oldChild)) {
-            // FIXME: Adjust 'Document::canReplaceChild' to return some additional detail (or an error message).
-            exceptionState.throwDOMException(HierarchyRequestError, "Failed to replace child.");
-            return false;
+    // TODO(esprehn): Remove this, sky should allow multiple top level elements.
+    if (isDocumentNode()) {
+        unsigned elementCount = 0;
+        if (newChild->isElementNode()) {
+            elementCount = 1;
+        } else if (newChild->isDocumentFragment()) {
+            for (Element* element = ElementTraversal::firstChild(*newChild); element; element = ElementTraversal::nextSibling(*element))
+                ++elementCount;
         }
-    } else if (!isChildTypeAllowed(*newChild)) {
-        exceptionState.throwDOMException(HierarchyRequestError, "Nodes of type '" + newChild->nodeName() + "' may not be inserted inside nodes of type '" + nodeName() + "'.");
-        return false;
+        if (elementCount > 1 || ((!oldChild || !oldChild->isElementNode()) && elementCount && document().documentElement())) {
+            exceptionState.throwDOMException(HierarchyRequestError, "Document can only contain one Element.");
+            return;
+        }
     }
-
-    return true;
-}
-
-bool ContainerNode::checkAcceptChildGuaranteedNodeTypes(const Node& newChild, ExceptionState& exceptionState) const
-{
-    ASSERT(isChildTypeAllowed(newChild));
-    if (newChild.contains(this)) {
-        exceptionState.throwDOMException(HierarchyRequestError, "The new child element contains the parent.");
-        return false;
-    }
-    return true;
 }
 
 PassRefPtr<Node> ContainerNode::insertBefore(PassRefPtr<Node> newChild, Node* refChild, ExceptionState& exceptionState)
 {
     // Check that this node is not "floating".
     // If it is, it can be deleted as a side effect of sending mutation events.
     ASSERT(refCount() || parentOrShadowHostNode());
 
     RefPtr<Node> protect(this);
 
     // 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.get(), 0, exceptionState)) {
-        if (exceptionState.hadException())
-            return nullptr;
-        return newChild;
-    }
+    checkAcceptChild(newChild.get(), 0, exceptionState);
+    if (exceptionState.hadException())
+        return nullptr;
+
     ASSERT(newChild);
 
     // NotFoundError: Raised if refChild is not a child of this node
     if (refChild->parentNode() != this) {
         exceptionState.throwDOMException(NotFoundError, "The node before which the new node is to be inserted is not a child of this node.");
         return nullptr;
     }
 
     // nothing to do
     if (refChild->previousSibling() == newChild || refChild == newChild)
         return newChild;
 
     RefPtr<Node> next = refChild;
 
     NodeVector targets;
     collectChildrenAndRemoveFromOldParent(*newChild, targets, exceptionState);
     if (exceptionState.hadException())
         return nullptr;
     if (targets.isEmpty())
         return newChild;
 
-    // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.

[ojan, 2014/11/18 02:46:57]

I think we still need this because of blur events, no? Or maybe we should make
blur events async?

[esprehn, 2014/11/18 03:01:40]

Ah yeah, damn. Maybe we should make them async?

-    if (!checkAcceptChildGuaranteedNodeTypes(*newChild, exceptionState)) {
-        if (exceptionState.hadException())
-            return nullptr;
-        return newChild;
-    }
-
     ChildListMutationScope mutation(*this);
     for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
         ASSERT(*it);
         Node& child = **it;
 
         // 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)
@@ skipping 62 matching lines @@
     if (oldChild == newChild) // nothing to do
         return oldChild;
 
     if (!oldChild) {
         exceptionState.throwDOMException(NotFoundError, "The node to be replaced is null.");
         return nullptr;
     }
 
     RefPtr<Node> child = oldChild;
 
-    // Make sure replacing the old child with the new is ok
-    if (!checkAcceptChild(newChild.get(), child.get(), exceptionState)) {
-        if (exceptionState.hadException())
-            return nullptr;
-        return child;
-    }
+    checkAcceptChild(newChild.get(), child.get(), exceptionState);
+    if (exceptionState.hadException())
+        return nullptr;
 
     // NotFoundError: Raised if oldChild is not a child of this node.
     if (child->parentNode() != this) {
         exceptionState.throwDOMException(NotFoundError, "The node to be replaced is not a child of this node.");
         return nullptr;
     }
 
     ChildListMutationScope mutation(*this);
 
     RefPtr<Node> next = child->nextSibling();
 
     // Remove the node we're replacing
     removeChild(child, exceptionState);
     if (exceptionState.hadException())
         return nullptr;
 
     if (next && (next->previousSibling() == newChild || next == newChild)) // nothing to do
         return child;
 
-    // Does this one more time because removeChild() fires a MutationEvent.

[ojan, 2014/11/18 02:46:57]

Ditto

-    if (!checkAcceptChild(newChild.get(), child.get(), exceptionState)) {
-        if (exceptionState.hadException())
-            return nullptr;
-        return child;
-    }
-
     NodeVector targets;
     collectChildrenAndRemoveFromOldParent(*newChild, targets, exceptionState);
     if (exceptionState.hadException())
         return nullptr;
 
-    // Does this yet another check because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.

[ojan, 2014/11/18 02:46:56]

Ditto

-    if (!checkAcceptChild(newChild.get(), child.get(), exceptionState)) {
-        if (exceptionState.hadException())
-            return nullptr;
-        return child;
-    }
-
     // Add the new child(ren)
     for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
         ASSERT(*it);
         Node& child = **it;
 
         // 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)
@@ skipping 213 matching lines @@
 }
 
 PassRefPtr<Node> ContainerNode::appendChild(PassRefPtr<Node> newChild, ExceptionState& exceptionState)
 {
     RefPtr<ContainerNode> protect(this);
 
     // Check that this node is not "floating".
     // If it is, it can be deleted as a side effect of sending mutation events.
     ASSERT(refCount() || parentOrShadowHostNode());
 
-    // Make sure adding the new child is ok
-    if (!checkAcceptChild(newChild.get(), 0, exceptionState)) {
-        if (exceptionState.hadException())
-            return nullptr;
-        return newChild;
-    }
+    checkAcceptChild(newChild.get(), 0, exceptionState);
+    if (exceptionState.hadException())
+        return nullptr;
+
     ASSERT(newChild);
 
     if (newChild == m_lastChild) // nothing to do
         return newChild;
 
     NodeVector targets;
     collectChildrenAndRemoveFromOldParent(*newChild, targets, exceptionState);
     if (exceptionState.hadException())
         return nullptr;
 
     if (targets.isEmpty())
         return newChild;
 
-    // We need this extra check because collectChildrenAndRemoveFromOldParent() can fire mutation events.

[ojan, 2014/11/18 02:46:56]

ditto

-    if (!checkAcceptChildGuaranteedNodeTypes(*newChild, exceptionState)) {
-        if (exceptionState.hadException())
-            return nullptr;
-        return newChild;
-    }
-
     // Now actually add the child(ren)
     ChildListMutationScope mutation(*this);
     for (NodeVector::const_iterator it = targets.begin(); it != targets.end(); ++it) {
         ASSERT(*it);
         Node& child = **it;
 
         // 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())
@@ skipping 397 matching lines @@
         return true;
 
     if (node->isElementNode() && toElement(node)->shadow())
         return true;
 
     return false;
 }
 #endif
 
 } // namespace blink