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

Source/core/dom/Node.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, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 364 matching lines @@
     LayoutObject* layoutObject = m_data.m_rareData->layoutObject();
     if (isElementNode())
         delete static_cast<ElementRareData*>(m_data.m_rareData);
     else
         delete static_cast<NodeRareData*>(m_data.m_rareData);
     m_data.m_layoutObject = layoutObject;
     clearFlag(HasRareDataFlag);
 }
 #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;
 }
 
 short Node::tabIndex() const
 {
     return 0;
 }
 
@@ skipping 99 matching lines @@
 
 PassRefPtrWillBeRawPtr<Node> Node::appendChild(PassRefPtrWillBeRawPtr<Node> newChild, ExceptionState& exceptionState)
 {
     if (isContainerNode())
         return toContainerNode(this)->appendChild(newChild, exceptionState);
 
     exceptionState.throwDOMException(HierarchyRequestError, "This node type does not support this method.");
     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())
         parent->removeChild(this, exceptionState);
 }
 
 void Node::normalize()
 {
     updateDistribution();
 
@@ skipping 1988 matching lines @@
 
 void showNodePath(const blink::Node* node)
 {
     if (node)
         node->showNodePathForThis();
     else
         fprintf(stderr, "Cannot showNodePath for (nil)\n");
 }
 
 #endif