Diff
Modified: trunk/LayoutTests/ChangeLog (147941 => 147942)
--- trunk/LayoutTests/ChangeLog 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/LayoutTests/ChangeLog 2013-04-08 19:10:43 UTC (rev 147942)
@@ -1,3 +1,34 @@
+2013-04-08 Andrei Bucur <[email protected]>
+
+ Simplify ContainerNode::removeChildren
+ https://bugs.webkit.org/show_bug.cgi?id=113517
+
+ Reviewed by Darin Adler.
+
+ The patch is based on the work made by Elliott Sprehn. He kindly agreed
+ for me to finalize the last bits and pieces of the fix.
+
+ Remove containerNode.html test since it was checking for an infinite
+ loop when adding DOM nodes inside a DOMNodeRemoved mutation event
+ handler, but we actually do want to allow an infinite loop here for
+ correctness and compatability with other browsers.
+
+ Also added mutation-during-innerHTML which checks that all nodes
+ are notified of being removed even if they were added during the
+ DOMNodeRemoved notification.
+
+ There's a new test range-remove-children-event that verifies the
+ ranges modified inside a mutation event handler remain consistent.
+
+ * fast/dom/MutationObserver/added-out-of-order-expected.txt:
+ * fast/dom/MutationObserver/added-out-of-order.html:
+ * fast/dom/Range/range-remove-children-event-expected.txt: Added.
+ * fast/dom/Range/range-remove-children-event.html: Added.
+ * fast/dom/containerNode-expected.txt: Removed.
+ * fast/dom/containerNode.html: Removed.
+ * fast/events/mutation-during-innerHTML-expected.txt: Added.
+ * fast/events/mutation-during-innerHTML.html: Added.
+
2013-04-08 Robert Hogan <[email protected]>
Unreviewed, rolling out r147850.
Modified: trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order-expected.txt (147941 => 147942)
--- trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order-expected.txt 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order-expected.txt 2013-04-08 19:10:43 UTC (rev 147942)
@@ -3,16 +3,15 @@
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-PASS mutations.length is 3
-PASS mutations[0].addedNodes.length is 0
-PASS mutations[0].removedNodes.length is 1
-PASS mutations[0].removedNodes[0].tagName is 'SPAN'
+PASS mutations.length is 2
+PASS mutations[0].addedNodes.length is 1
+PASS mutations[0].removedNodes.length is 0
+PASS mutations[0].addedNodes[0].tagName is "DIV"
PASS mutations[1].addedNodes.length is 1
-PASS mutations[1].removedNodes.length is 0
-PASS mutations[1].addedNodes[0].tagName is 'DIV'
-PASS mutations[2].addedNodes.length is 1
-PASS mutations[2].removedNodes.length is 0
-PASS mutations[2].addedNodes[0].nodeValue is 'hello world'
+PASS mutations[1].removedNodes.length is 2
+PASS mutations[1].addedNodes[0].nodeValue is "hello world"
+PASS mutations[1].removedNodes[0].tagName is "SPAN"
+PASS mutations[1].removedNodes[1].tagName is "DIV"
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order.html (147941 => 147942)
--- trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order.html 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order.html 2013-04-08 19:10:43 UTC (rev 147942)
@@ -16,15 +16,14 @@
sandbox.textContent = 'hello world';
var mutations = observer.takeRecords();
-shouldBe("mutations.length", "3");
-shouldBe("mutations[0].addedNodes.length", "0");
-shouldBe("mutations[0].removedNodes.length", "1");
-shouldBe("mutations[0].removedNodes[0].tagName", "'SPAN'");
+shouldBe("mutations.length", "2");
+shouldBe("mutations[0].addedNodes.length", "1");
+shouldBe("mutations[0].removedNodes.length", "0");
+shouldBeEqualToString("mutations[0].addedNodes[0].tagName", "DIV");
shouldBe("mutations[1].addedNodes.length", "1");
-shouldBe("mutations[1].removedNodes.length", "0");
-shouldBe("mutations[1].addedNodes[0].tagName", "'DIV'");
-shouldBe("mutations[2].addedNodes.length", "1");
-shouldBe("mutations[2].removedNodes.length", "0");
-shouldBe("mutations[2].addedNodes[0].nodeValue", "'hello world'");
+shouldBe("mutations[1].removedNodes.length", "2");
+shouldBeEqualToString("mutations[1].addedNodes[0].nodeValue", "hello world");
+shouldBeEqualToString("mutations[1].removedNodes[0].tagName", "SPAN");
+shouldBeEqualToString("mutations[1].removedNodes[1].tagName", "DIV");
</script>
<script src=""
Added: trunk/LayoutTests/fast/dom/Range/range-remove-children-event-expected.txt (0 => 147942)
--- trunk/LayoutTests/fast/dom/Range/range-remove-children-event-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/dom/Range/range-remove-children-event-expected.txt 2013-04-08 19:10:43 UTC (rev 147942)
@@ -0,0 +1,10 @@
+Test ranges remain valid when modified inside a MutationEvent handler.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS range.startContainer.tagName is "DIV"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/dom/Range/range-remove-children-event.html (0 => 147942)
--- trunk/LayoutTests/fast/dom/Range/range-remove-children-event.html (rev 0)
+++ trunk/LayoutTests/fast/dom/Range/range-remove-children-event.html 2013-04-08 19:10:43 UTC (rev 147942)
@@ -0,0 +1,39 @@
+<html>
+<head>
+ <script src=""
+</head>
+<body>
+ <div id="content">
+ <span id="innerContent">A</span>
+ </div>
+ <script>
+ window.jsTestIsAsync = true;
+ description("Test ranges remain valid when modified inside a MutationEvent handler.");
+
+ var content = document.getElementById("content");
+ var inserted = false;
+ var range = document.createRange();
+ range.setStart(content, 0);
+ range.setEnd(content, 0);
+ content.addEventListener('DOMNodeRemoved', function() {
+ if (!inserted) {
+ var newChild = document.createElement('p');
+ content.appendChild(newChild);
+ inserted = true;
+
+ range.setStart(newChild);
+ range.setEnd(newChild);
+
+ setTimeout(checkRange, 0);
+ }
+ });
+ content.innerHTML = "";
+
+ function checkRange() {
+ shouldBeEqualToString("range.startContainer.tagName", "DIV");
+ finishJSTest();
+ }
+ </script>
+ <script src=""
+</body>
+</html>
Deleted: trunk/LayoutTests/fast/dom/containerNode-expected.txt (147941 => 147942)
--- trunk/LayoutTests/fast/dom/containerNode-expected.txt 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/LayoutTests/fast/dom/containerNode-expected.txt 2013-04-08 19:10:43 UTC (rev 147942)
@@ -1 +0,0 @@
-PASS: No infinite loop.
Deleted: trunk/LayoutTests/fast/dom/containerNode.html (147941 => 147942)
--- trunk/LayoutTests/fast/dom/containerNode.html 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/LayoutTests/fast/dom/containerNode.html 2013-04-08 19:10:43 UTC (rev 147942)
@@ -1,46 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
- <script type="text/_javascript_">
- function log(msg)
- {
- document.body.appendChild(document.createTextNode(msg));
- }
-
- function appendItem(list, caption)
- {
- var item = document.createElement('li');
- item.appendChild(document.createTextNode(caption));
- list.appendChild(item);
- }
-
- function runTests()
- {
- if (window.testRunner)
- testRunner.dumpAsText();
-
- var fragment = document.createDocumentFragment();
- var list = document.createElement('ul');
- var i;
- for (i = 0; i < 5; i++)
- appendItem(list, 'item ' + i);
-
- fragment.appendChild(list);
- document.addEventListener("DOMNodeRemoved", function() {
- appendItem(list, 'item ' + i++);
- }, false);
-
- document.body.appendChild(fragment);
- list.textContent = '';
-
- if (list.childNodes.length == 0)
- log('PASS: No infinite loop.')
- else
- log('FAIL: Has too many children.')
- }
- </script>
-</head>
-<body _onload_="runTests();">
-
-</body>
-</html>
Added: trunk/LayoutTests/fast/events/mutation-during-innerHTML-expected.txt (0 => 147942)
--- trunk/LayoutTests/fast/events/mutation-during-innerHTML-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/mutation-during-innerHTML-expected.txt 2013-04-08 19:10:43 UTC (rev 147942)
@@ -0,0 +1,3 @@
+Test that nodes added during DOMNodeRemoved from innerHTML are notified of their eventual removal.
+
+PASS
Added: trunk/LayoutTests/fast/events/mutation-during-innerHTML.html (0 => 147942)
--- trunk/LayoutTests/fast/events/mutation-during-innerHTML.html (rev 0)
+++ trunk/LayoutTests/fast/events/mutation-during-innerHTML.html 2013-04-08 19:10:43 UTC (rev 147942)
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+
+<p>Test that nodes added during DOMNodeRemoved from innerHTML are notified of their eventual removal.</p>
+
+<div id="container">
+ <div id="first"></div>
+</div>
+
+<script>
+if (window.testRunner)
+ testRunner.dumpAsText();
+
+var container = document.getElementById('container');
+var second = document.createElement('div');
+second.id = 'second';
+
+container.addEventListener('DOMNodeRemoved', function(event) {
+ if (event.target.id == 'first')
+ container.appendChild(second);
+ if (event.target.id == 'second')
+ document.body.appendChild(document.createTextNode('PASS'));
+});
+
+container.innerHTML = '';
+</script>
Modified: trunk/Source/WebCore/ChangeLog (147941 => 147942)
--- trunk/Source/WebCore/ChangeLog 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/Source/WebCore/ChangeLog 2013-04-08 19:10:43 UTC (rev 147942)
@@ -1,3 +1,42 @@
+2013-04-08 Andrei Bucur <[email protected]>
+
+ Simplify ContainerNode::removeChildren
+ https://bugs.webkit.org/show_bug.cgi?id=113517
+
+ Reviewed by Darin Adler.
+
+ The patch is based on the work made by Elliott Sprehn. He kindly agreed
+ for me to finalize the last bits and pieces of the fix.
+
+ Simplify ContainerNode::removeChildren by merging the loops and removing
+ willRemoveChildren. This removes two traversals of the children, avoids
+ refing and derefing all the children once, avoids allocating a second
+ NodeVector of children, and means we detach() in the same order as
+ normal removal.
+
+ This does mean you can get into an infinite loop with DOMNodeRemoved
+ listeners by continously adding nodes but this is true in all other browsers
+ and the current behavior is bad because it means you don't get notified
+ of nodes added during removal (which other browsers do notify of). This
+ patch removes the containerNode.html test that originally tested for this
+ infinite loop and adds a new one that tests that all nodes get notified.
+
+ This makes PerformanceTests/Parser/innerHTML-setter.html 2-6% faster.
+
+ There's also a new test verifying ranges remain consistent if modified
+ inside an mutation event handler. Without the patch it's possible to create
+ a range with boundaries outside of the DOM tree.
+
+ Tests: fast/dom/Range/range-remove-children-event.html
+ fast/events/mutation-during-innerHTML.html
+
+ * dom/ContainerNode.cpp:
+ (WebCore::ContainerNode::removeChildren):
+ * dom/Document.cpp:
+ * dom/Document.h: nodeChildrenWillBeRemoved is not needed any more.
+ * dom/Range.cpp:
+ * dom/Range.h: nodeChildrenWillBeRemoved is not needed any more.
+
2013-04-06 Simon Fraser <[email protected]>
Remove some #includes in headers in rendering code
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (147941 => 147942)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2013-04-08 19:10:43 UTC (rev 147942)
@@ -454,26 +454,6 @@
ChildFrameDisconnector(child).disconnect();
}
-static void willRemoveChildren(ContainerNode* container)
-{
- NodeVector children;
- getChildNodes(container, children);
-
- container->document()->nodeChildrenWillBeRemoved(container);
-
- ChildListMutationScope mutation(container);
- for (NodeVector::const_iterator it = children.begin(); it != children.end(); ++it) {
- Node* child = it->get();
- mutation.willRemoveChild(child);
- child->notifyMutationObserversNodeWillDetach();
-
- // fire removed from document mutation events.
- dispatchChildRemovalEvents(child);
- }
-
- ChildFrameDisconnector(container).disconnect(ChildFrameDisconnector::DescendantsOnly);
-}
-
void ContainerNode::disconnectDescendantFrames()
{
ChildFrameDisconnector(this).disconnect();
@@ -595,55 +575,51 @@
// The container node can be removed from event handlers.
RefPtr<ContainerNode> protect(this);
- // exclude this node when looking for removed focusedNode since only children will be removed
+ // Exclude this node when looking for removed focusedNode since only children will be removed.
document()->removeFocusedNodeOfSubtree(this, true);
#if ENABLE(FULLSCREEN_API)
document()->removeFullScreenElementOfSubtree(this, true);
#endif
- // Do any prep work needed before actually starting to detach
- // and remove... e.g. stop loading frames, fire unload events.
- willRemoveChildren(protect.get());
-
- Vector<RefPtr<Node>, 10> removedChildren;
+ ChildListMutationScope mutation(this);
+ NodeVector removedChildren;
{
WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
- {
- NoEventDispatchAssertion assertNoEventDispatch;
- removedChildren.reserveInitialCapacity(childNodeCount());
- while (RefPtr<Node> n = m_firstChild) {
- 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().
- n->setPreviousSibling(0);
- n->setNextSibling(0);
- n->setParentOrShadowHostNode(0);
- document()->adoptIfNeeded(n.get());
+ while (RefPtr<Node> child = m_firstChild) {
+ // First dispatch the mutation events if any.
+ // Unfortunately it's possible for this to be called more than once for a node
+ // because of the nature of mutation events (if the node is moved further in the child list
+ // by an event handler).
+ dispatchChildRemovalEvents(child.get());
+ ChildFrameDisconnector(child.get()).disconnect();
- m_firstChild = next;
- if (n == m_lastChild)
- m_lastChild = 0;
- removedChildren.append(n.release());
- }
+ // Mutation or unload events could have moved this child.
+ if (child != m_firstChild)
+ continue;
- // 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();
- }
+ // We can't use a bulk version of document()->nodeWillBeRemoved() before the removal loop.
+ // We need to call document()->nodeWillBeRemoved() on each node in case the node was created
+ // by a mutation event handler.
+ // document()->nodeWillbeRemoved() may modify the children list so we may need to retry.
+ document()->nodeWillBeRemoved(child.get());
+ if (child != m_firstChild)
+ continue;
+
+ // Notify the mutation observers.
+ mutation.willRemoveChild(child.get());
+ child->notifyMutationObserversNodeWillDetach();
+
+ removeBetween(0, child->nextSibling(), child.get());
+ removedChildren.append(child.release());
}
+ // FIXME: We could avoid walking all the children twice by calling
+ // notify inside the loop and childrenChanged after but that would mean
+ // calling childrenChanged in a different order than all other methods.
+ // Figure out if this is safe.
childrenChanged(false, 0, 0, -static_cast<int>(removedChildren.size()));
-
for (size_t i = 0; i < removedChildren.size(); ++i)
ChildNodeRemovalNotifier(this).notify(removedChildren[i].get());
}
Modified: trunk/Source/WebCore/dom/Document.cpp (147941 => 147942)
--- trunk/Source/WebCore/dom/Document.cpp 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/Source/WebCore/dom/Document.cpp 2013-04-08 19:10:43 UTC (rev 147942)
@@ -3568,29 +3568,6 @@
}
}
-void Document::nodeChildrenWillBeRemoved(ContainerNode* container)
-{
- if (!m_ranges.isEmpty()) {
- HashSet<Range*>::const_iterator end = m_ranges.end();
- for (HashSet<Range*>::const_iterator it = m_ranges.begin(); it != end; ++it)
- (*it)->nodeChildrenWillBeRemoved(container);
- }
-
- HashSet<NodeIterator*>::const_iterator nodeIteratorsEnd = m_nodeIterators.end();
- for (HashSet<NodeIterator*>::const_iterator it = m_nodeIterators.begin(); it != nodeIteratorsEnd; ++it) {
- for (Node* n = container->firstChild(); n; n = n->nextSibling())
- (*it)->nodeWillBeRemoved(n);
- }
-
- if (Frame* frame = this->frame()) {
- for (Node* n = container->firstChild(); n; n = n->nextSibling()) {
- frame->eventHandler()->nodeWillBeRemoved(n);
- frame->selection()->nodeWillBeRemoved(n);
- frame->page()->dragCaretController()->nodeWillBeRemoved(n);
- }
- }
-}
-
void Document::nodeWillBeRemoved(Node* n)
{
HashSet<NodeIterator*>::const_iterator nodeIteratorsEnd = m_nodeIterators.end();
Modified: trunk/Source/WebCore/dom/Document.h (147941 => 147942)
--- trunk/Source/WebCore/dom/Document.h 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/Source/WebCore/dom/Document.h 2013-04-08 19:10:43 UTC (rev 147942)
@@ -724,8 +724,6 @@
void detachRange(Range*);
void updateRangesAfterChildrenChanged(ContainerNode*);
- // nodeChildrenWillBeRemoved is used when removing all node children at once.
- void nodeChildrenWillBeRemoved(ContainerNode*);
// nodeWillBeRemoved is only safe when removing one node at a time.
void nodeWillBeRemoved(Node*);
bool canReplaceChild(Node* newChild, Node* oldChild);
Modified: trunk/Source/WebCore/dom/Range.cpp (147941 => 147942)
--- trunk/Source/WebCore/dom/Range.cpp 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/Source/WebCore/dom/Range.cpp 2013-04-08 19:10:43 UTC (rev 147942)
@@ -1732,31 +1732,6 @@
boundaryNodeChildrenChanged(m_end, container);
}
-static inline void boundaryNodeChildrenWillBeRemoved(RangeBoundaryPoint& boundary, ContainerNode* container)
-{
- for (Node* nodeToBeRemoved = container->firstChild(); nodeToBeRemoved; nodeToBeRemoved = nodeToBeRemoved->nextSibling()) {
- if (boundary.childBefore() == nodeToBeRemoved) {
- boundary.setToStartOfNode(container);
- return;
- }
-
- for (Node* n = boundary.container(); n; n = n->parentNode()) {
- if (n == nodeToBeRemoved) {
- boundary.setToStartOfNode(container);
- return;
- }
- }
- }
-}
-
-void Range::nodeChildrenWillBeRemoved(ContainerNode* container)
-{
- ASSERT(container);
- ASSERT(container->document() == m_ownerDocument);
- boundaryNodeChildrenWillBeRemoved(m_start, container);
- boundaryNodeChildrenWillBeRemoved(m_end, container);
-}
-
static inline void boundaryNodeWillBeRemoved(RangeBoundaryPoint& boundary, Node* nodeToBeRemoved)
{
if (boundary.childBefore() == nodeToBeRemoved) {
Modified: trunk/Source/WebCore/dom/Range.h (147941 => 147942)
--- trunk/Source/WebCore/dom/Range.h 2013-04-08 19:06:57 UTC (rev 147941)
+++ trunk/Source/WebCore/dom/Range.h 2013-04-08 19:10:43 UTC (rev 147942)
@@ -128,7 +128,6 @@
FloatRect boundingRect() const;
void nodeChildrenChanged(ContainerNode*);
- void nodeChildrenWillBeRemoved(ContainerNode*);
void nodeWillBeRemoved(Node*);
void textInserted(Node*, unsigned offset, unsigned length);
