Changeset 148754 in webkit

Timestamp:

Apr 19, 2013 10:25:54 AM (11 years ago)

Author:

abucur@adobe.com

Message:

ContainerNode::removeChildren should first detach the children then remove them
​https://bugs.webkit.org/show_bug.cgi?id=113433

Reviewed by Ryosuke Niwa.

Currently, ContainerNode::removeChildren initially removes the nodes from the DOM tree and then
calls detach() on each of them. This is anti-intuitive and can lead to subtle bugs because the
detached renderers are not backed by a valid DOM tree any more. With the patch, the nodes are first
detached and then removed from the DOM.
The patch also lets the tree in a consistent state after each node removal by clearing the previous
sibling pointer of the node following the one removed.
I haven't found any proof the performance will get worse if the detachment happens when the children
are still in the DOM tree.

Tests: No changed visible functionality.

• dom/ContainerNode.cpp:

(WebCore::ContainerNode::removeChildren):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/ContainerNode.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-04-19  Andrei Bucur  <abucur@adobe.com>
+
+        ContainerNode::removeChildren should first detach the children then remove them
+        https://bugs.webkit.org/show_bug.cgi?id=113433
+
+        Reviewed by Ryosuke Niwa.
+
+        Currently, ContainerNode::removeChildren initially removes the nodes from the DOM tree and then
+        calls detach() on each of them. This is anti-intuitive and can lead to subtle bugs because the
+        detached renderers are not backed by a valid DOM tree any more. With the patch, the nodes are first
+        detached and then removed from the DOM.
+        The patch also lets the tree in a consistent state after each node removal by clearing the previous
+        sibling pointer of the node following the one removed.
+        I haven't found any proof the performance will get worse if the detachment happens when the children
+        are still in the DOM tree.
+
+        Tests: No changed visible functionality.
+
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeChildren):
+
 2013-04-19  Dirk Schulze  <krit@webkit.org>
 

trunk/Source/WebCore/dom/ContainerNode.cpp

@@ -617 +617 @@
                 Node* next = n->nextSibling();
 
-                // Remove the node from the tree before calling detach or removedFromDocument (4427024, 4129744).
-                // removeChild() does this after calling detach(). There is no explanation for
-                // this discrepancy between removeChild() and its optimized version removeChildren().
+                if (n->attached())
+                    n->detach();
+
                 n->setPreviousSibling(0);
                 n->setNextSibling(0);
@@ -628 +628 @@
                 if (n == m_lastChild)
                     m_lastChild = 0;
+                else
+                    m_firstChild->setPreviousSibling(0);
+
                 removedChildren.append(n.release());
-            }
-
-            // Detach the nodes only after properly removed from the tree because
-            // a. detaching requires a proper DOM tree (for counters and quotes for
-            // example) and during the previous loop the next sibling still points to
-            // the node being removed while the node being removed does not point back
-            // and does not point to the same parent as its next sibling.
-            // b. destroying Renderers of standalone nodes is sometimes faster.
-            for (size_t i = 0; i < removedChildren.size(); ++i) {
-                Node* removedChild = removedChildren[i].get();
-                if (removedChild->attached())
-                    removedChild->detach();
             }
         }