Implement DOM: prepend, append, before, after & replaceWith

Source/core/dom/Node.cpp

Index: Source/core/dom/Node.cpp
diff --git a/Source/core/dom/Node.cpp b/Source/core/dom/Node.cpp
index 56fb995d78a7033954fdda8b5b547b0d8d05500c..17f1020cc98f3ee50ba34fc6bc38dfd28bbbd2d1 100644
--- a/Source/core/dom/Node.cpp
+++ b/Source/core/dom/Node.cpp
@@ -382,6 +382,51 @@ void Node::clearRareData()
 }
 #endif
 
+static bool isNodeInNodes(const Node& node, const HeapVector<NodeOrString>& nodes)

[philipj_slow, 2015/06/26 14:13:25]

Maybe move this block of statics to just before Node::prepend where they're
used?

[Paritosh Kumar, 2015/06/30 10:49:09]

Done.

+{
+    for (NodeOrString nodeOrString : nodes) {

[philipj_slow, 2015/06/26 14:13:26]

I guess this should be "for (const NodeOrString& nodeOrString : nodes)" like in
the other loop. My bad.

[Paritosh Kumar, 2015/06/30 10:49:09]

Ohh, sorry. Thanks.

+        if (nodeOrString.isNode() && nodeOrString.getAsNode() == &node)
+            return true;
+    }
+    return false;
+}
+
+static Node* viablePreviousSibling(const Node& node, const HeapVector<NodeOrString>& nodes)
+{
+    for (Node* sibling = node.previousSibling(); sibling; sibling = sibling->previousSibling()) {
+        if (!isNodeInNodes(*sibling, nodes))
+            return sibling;
+    }
+    return nullptr;
+}
+
+static Node* viableNextSibling(const Node& node, const HeapVector<NodeOrString>& nodes)
+{
+    for (Node* sibling = node.nextSibling(); sibling; sibling = sibling->nextSibling()) {
+        if (!isNodeInNodes(*sibling, nodes))
+            return sibling;
+    }
+    return nullptr;
+}
+
+static PassRefPtrWillBeRawPtr<Node> nodeOrStringToNode(const NodeOrString& nodeOrString, Document& document)
+{
+    if (nodeOrString.isNode())
+        return nodeOrString.getAsNode();
+    return Text::create(document, nodeOrString.getAsString());
+}
+
+static PassRefPtrWillBeRawPtr<Node> convertNodesIntoNode(const HeapVector<NodeOrString>& nodes, Document& document)
+{
+    if (nodes.size() == 1)
+        return nodeOrStringToNode(nodes[0], document);
+
+    RefPtrWillBeRawPtr<Node> node = DocumentFragment::create(document);

[philipj_slow, 2015/06/26 14:13:26]

For clarity I would name this fragment instead of node.

[Paritosh Kumar, 2015/06/30 10:49:08]

Done.

+    for (const NodeOrString& nodeOrString : nodes)
+        node->appendChild(nodeOrStringToNode(nodeOrString, document), ASSERT_NO_EXCEPTION);
+    return node.release();
+}
+
 Node* Node::toNode()
 {
     return this;
@@ -501,6 +546,49 @@ PassRefPtrWillBeRawPtr<Node> Node::appendChild(PassRefPtrWillBeRawPtr<Node> newC
     return nullptr;
 }
 
+void Node::prepend(const HeapVector<NodeOrString>& nodes, ExceptionState& exceptionState)
+{
+    this->insertBefore(convertNodesIntoNode(nodes, document()), this->firstChild(), exceptionState);

[philipj_slow, 2015/06/26 14:13:26]

Is "this->" needed?

[Paritosh Kumar, 2015/06/30 10:49:09]

No.

[philipj_slow, 2015/07/02 09:20:01]

How about the "this->" at the beginning of the line here and in append()?

+}
+
+void Node::append(const HeapVector<NodeOrString>& nodes, ExceptionState& exceptionState)
+{
+    this->appendChild(convertNodesIntoNode(nodes, document()), exceptionState);
+}
+
+void Node::before(const HeapVector<NodeOrString>& nodes, ExceptionState& exceptionState)
+{
+    Node* parent = parentNode();
+    if (!parent)
+        return;
+    Node* viablePreviousSiblingNode = viablePreviousSibling(*this, nodes);

[philipj_slow, 2015/06/26 14:13:26]

Can this bit be written like the spec, with an |viablePreviousSiblingNode =
viablePreviousSiblingNode->nextSibling() : parent->firstChild()|? That should
result in a single call to insertBefore and thus a single call to
convertNodesIntoNode.

If there was a bug with the missing nextSibling() call, make sure there's a test
that fails if that bug is present.

On another topic, if you think it would look nicer (matching the spec) you might
consider renaming the variable to viablePreviousSibling and the helper method to
findViablePreviousSibling() or similar.

[Paritosh Kumar, 2015/06/30 10:49:09]

Changed with conditional. Yes, this should be
viablePreviousSibling->nextSibling().

[philipj_slow, 2015/07/02 09:20:01]

I see that you changed the before.html test, but can you confirm that it would
fail with the original bug in place?

+    if (!viablePreviousSiblingNode)
+        parent->insertBefore(convertNodesIntoNode(nodes, document()), parent->firstChild(), exceptionState);
+    else
+        parent->insertBefore(convertNodesIntoNode(nodes, document()), viablePreviousSiblingNode, exceptionState);
+}
+
+void Node::after(const HeapVector<NodeOrString>& nodes, ExceptionState& exceptionState)
+{
+    Node* parent = parentNode();
+    if (!parent)
+        return;
+    Node* viableNextSiblingNode = viableNextSibling(*this, nodes);
+    parent->insertBefore(convertNodesIntoNode(nodes, document()), viableNextSiblingNode, exceptionState);
+}
+
+void Node::replaceWith(const HeapVector<NodeOrString>& nodes, ExceptionState& exceptionState)
+{
+    Node* parent = parentNode();
+    if (!parent)
+        return;
+    Node* viableNextSiblingNode = viableNextSibling(*this, nodes);
+    if (parent == parentNode())

[philipj_slow, 2015/06/26 14:13:26]

The spec says "If context object’s parent is parent, replace the context object
with node within parent" here but it seems very strange. Nothing between the
"parent = parentNode()" and this "parent == parentNode()" check could actually
have changed parent or parentNode(), could it? Spec bug?

[Paritosh Kumar, 2015/06/30 10:49:09]

Hmmm, This looks very strange but @Annevk has taken this from
https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724...
as per his comment on
https://github.com/inexorabletash/polyfill/commit/a9f17a7bacc588de674832b4724...,
"I like @inexorabletash's this.parent === parent check to preserve the replace
mutation records where possible."

But I'm not sure, this line is required in our implementation.

[philipj_slow, 2015/07/02 09:20:01]

Discussed with Anne on IRC:
http://krijnhoetmer.nl/irc-logs/whatwg/20150702#l-440

This is because |this| could have been inserted into the fragment by the
conversion. This makes it very important that convertNodesIntoNode is called
before the check, so if these steps can be structured more like the spec that
would probably do it.

Please also be sure to write a test that fails with the current code but passes
with the fixed code.

+        parent->replaceChild(convertNodesIntoNode(nodes, document()), this, exceptionState);
+    else
+        parent->insertBefore(convertNodesIntoNode(nodes, document()), viableNextSiblingNode, exceptionState);
+}
+
 void Node::remove(ExceptionState& exceptionState)
 {
     if (ContainerNode* parent = parentNode())