Changeset 147942 in webkit


Ignore:
Timestamp:
Apr 8, 2013 12:10:43 PM (11 years ago)
Author:
abucur@adobe.com
Message:

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.

Source/WebCore:

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.

LayoutTests:

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.
Location:
trunk
Files:
4 added
2 deleted
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r147939 r147942  
     12013-04-08  Andrei Bucur  <abucur@adobe.com>
     2
     3        Simplify ContainerNode::removeChildren
     4        https://bugs.webkit.org/show_bug.cgi?id=113517
     5
     6        Reviewed by Darin Adler.
     7
     8        The patch is based on the work made by Elliott Sprehn. He kindly agreed
     9        for me to finalize the last bits and pieces of the fix.
     10
     11        Remove containerNode.html test since it was checking for an infinite
     12        loop when adding DOM nodes inside a DOMNodeRemoved mutation event
     13        handler, but we actually do want to allow an infinite loop here for
     14        correctness and compatability with other browsers.
     15
     16        Also added mutation-during-innerHTML which checks that all nodes
     17        are notified of being removed even if they were added during the
     18        DOMNodeRemoved notification.
     19
     20        There's a new test range-remove-children-event that verifies the
     21        ranges modified inside a mutation event handler remain consistent.
     22
     23        * fast/dom/MutationObserver/added-out-of-order-expected.txt:
     24        * fast/dom/MutationObserver/added-out-of-order.html:
     25        * fast/dom/Range/range-remove-children-event-expected.txt: Added.
     26        * fast/dom/Range/range-remove-children-event.html: Added.
     27        * fast/dom/containerNode-expected.txt: Removed.
     28        * fast/dom/containerNode.html: Removed.
     29        * fast/events/mutation-during-innerHTML-expected.txt: Added.
     30        * fast/events/mutation-during-innerHTML.html: Added.
     31
    1322013-04-08  Robert Hogan  <robert@webkit.org>
    233

  • trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order-expected.txt

    r137662 r147942  
    44
    55
    6 PASS mutations.length is 3
    7 PASS mutations[0].addedNodes.length is 0
    8 PASS mutations[0].removedNodes.length is 1
    9 PASS mutations[0].removedNodes[0].tagName is 'SPAN'
     6PASS mutations.length is 2
     7PASS mutations[0].addedNodes.length is 1
     8PASS mutations[0].removedNodes.length is 0
     9PASS mutations[0].addedNodes[0].tagName is "DIV"
    1010PASS mutations[1].addedNodes.length is 1
    11 PASS mutations[1].removedNodes.length is 0
    12 PASS mutations[1].addedNodes[0].tagName is 'DIV'
    13 PASS mutations[2].addedNodes.length is 1
    14 PASS mutations[2].removedNodes.length is 0
    15 PASS mutations[2].addedNodes[0].nodeValue is 'hello world'
     11PASS mutations[1].removedNodes.length is 2
     12PASS mutations[1].addedNodes[0].nodeValue is "hello world"
     13PASS mutations[1].removedNodes[0].tagName is "SPAN"
     14PASS mutations[1].removedNodes[1].tagName is "DIV"
    1615PASS successfullyParsed is true
    1716

  • trunk/LayoutTests/fast/dom/MutationObserver/added-out-of-order.html

    r143386 r147942  
    1717
    1818var mutations = observer.takeRecords();
    19 shouldBe("mutations.length", "3");
    20 shouldBe("mutations[0].addedNodes.length", "0");
    21 shouldBe("mutations[0].removedNodes.length", "1");
    22 shouldBe("mutations[0].removedNodes[0].tagName", "'SPAN'");
     19shouldBe("mutations.length", "2");
     20shouldBe("mutations[0].addedNodes.length", "1");
     21shouldBe("mutations[0].removedNodes.length", "0");
     22shouldBeEqualToString("mutations[0].addedNodes[0].tagName", "DIV");
    2323shouldBe("mutations[1].addedNodes.length", "1");
    24 shouldBe("mutations[1].removedNodes.length", "0");
    25 shouldBe("mutations[1].addedNodes[0].tagName", "'DIV'");
    26 shouldBe("mutations[2].addedNodes.length", "1");
    27 shouldBe("mutations[2].removedNodes.length", "0");
    28 shouldBe("mutations[2].addedNodes[0].nodeValue", "'hello world'");
     24shouldBe("mutations[1].removedNodes.length", "2");
     25shouldBeEqualToString("mutations[1].addedNodes[0].nodeValue", "hello world");
     26shouldBeEqualToString("mutations[1].removedNodes[0].tagName", "SPAN");
     27shouldBeEqualToString("mutations[1].removedNodes[1].tagName", "DIV");
    2928</script>
    3029<script src="../../js/resources/js-test-post.js"></script>

  • trunk/Source/WebCore/ChangeLog

    r147940 r147942  
     12013-04-08  Andrei Bucur  <abucur@adobe.com>
     2
     3        Simplify ContainerNode::removeChildren
     4        https://bugs.webkit.org/show_bug.cgi?id=113517
     5
     6        Reviewed by Darin Adler.
     7
     8        The patch is based on the work made by Elliott Sprehn. He kindly agreed
     9        for me to finalize the last bits and pieces of the fix.
     10
     11        Simplify ContainerNode::removeChildren by merging the loops and removing
     12        willRemoveChildren. This removes two traversals of the children, avoids
     13        refing and derefing all the children once, avoids allocating a second
     14        NodeVector of children, and means we detach() in the same order as
     15        normal removal.
     16
     17        This does mean you can get into an infinite loop with DOMNodeRemoved
     18        listeners by continously adding nodes but this is true in all other browsers
     19        and the current behavior is bad because it means you don't get notified
     20        of nodes added during removal (which other browsers do notify of). This
     21        patch removes the containerNode.html test that originally tested for this
     22        infinite loop and adds a new one that tests that all nodes get notified.
     23
     24        This makes PerformanceTests/Parser/innerHTML-setter.html 2-6% faster.
     25
     26        There's also a new test verifying ranges remain consistent if modified
     27        inside an mutation event handler. Without the patch it's possible to create
     28        a range with boundaries outside of the DOM tree.
     29
     30        Tests: fast/dom/Range/range-remove-children-event.html
     31               fast/events/mutation-during-innerHTML.html
     32
     33        * dom/ContainerNode.cpp:
     34        (WebCore::ContainerNode::removeChildren):
     35        * dom/Document.cpp:
     36        * dom/Document.h: nodeChildrenWillBeRemoved is not needed any more.
     37        * dom/Range.cpp:
     38        * dom/Range.h: nodeChildrenWillBeRemoved is not needed any more.
     39
    1402013-04-06  Simon Fraser  <simon.fraser@apple.com>
    241

  • trunk/Source/WebCore/dom/ContainerNode.cpp

    r147832 r147942  
    455455}
    456456
    457 static void willRemoveChildren(ContainerNode* container)
    458 {
    459     NodeVector children;
    460     getChildNodes(container, children);
    461 
    462     container->document()->nodeChildrenWillBeRemoved(container);
    463 
    464     ChildListMutationScope mutation(container);
    465     for (NodeVector::const_iterator it = children.begin(); it != children.end(); ++it) {
    466         Node* child = it->get();
    467         mutation.willRemoveChild(child);
    468         child->notifyMutationObserversNodeWillDetach();
    469 
    470         // fire removed from document mutation events.
    471         dispatchChildRemovalEvents(child);
    472     }
    473 
    474     ChildFrameDisconnector(container).disconnect(ChildFrameDisconnector::DescendantsOnly);
    475 }
    476 
    477457void ContainerNode::disconnectDescendantFrames()
    478458{
    … …  
    596576    RefPtr<ContainerNode> protect(this);
    597577
    598     // exclude this node when looking for removed focusedNode since only children will be removed
     578    // Exclude this node when looking for removed focusedNode since only children will be removed.
    599579    document()->removeFocusedNodeOfSubtree(this, true);
    600580
    … …  
    603583#endif
    604584
    605     // Do any prep work needed before actually starting to detach
    606     // and remove... e.g. stop loading frames, fire unload events.
    607     willRemoveChildren(protect.get());
    608 
    609     Vector<RefPtr<Node>, 10> removedChildren;
     585    ChildListMutationScope mutation(this);
     586    NodeVector removedChildren;
    610587    {
    611588        WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
    612         {
    613             NoEventDispatchAssertion assertNoEventDispatch;
    614             removedChildren.reserveInitialCapacity(childNodeCount());
    615             while (RefPtr<Node> n = m_firstChild) {
    616                 Node* next = n->nextSibling();
    617 
    618                 // Remove the node from the tree before calling detach or removedFromDocument (4427024, 4129744).
    619                 // removeChild() does this after calling detach(). There is no explanation for
    620                 // this discrepancy between removeChild() and its optimized version removeChildren().
    621                 n->setPreviousSibling(0);
    622                 n->setNextSibling(0);
    623                 n->setParentOrShadowHostNode(0);
    624                 document()->adoptIfNeeded(n.get());
    625 
    626                 m_firstChild = next;
    627                 if (n == m_lastChild)
    628                     m_lastChild = 0;
    629                 removedChildren.append(n.release());
    630             }
    631 
    632             // Detach the nodes only after properly removed from the tree because
    633             // a. detaching requires a proper DOM tree (for counters and quotes for
    634             // example) and during the previous loop the next sibling still points to
    635             // the node being removed while the node being removed does not point back
    636             // and does not point to the same parent as its next sibling.
    637             // b. destroying Renderers of standalone nodes is sometimes faster.
    638             for (size_t i = 0; i < removedChildren.size(); ++i) {
    639                 Node* removedChild = removedChildren[i].get();
    640                 if (removedChild->attached())
    641                     removedChild->detach();
    642             }
     589
     590        while (RefPtr<Node> child = m_firstChild) {
     591            // First dispatch the mutation events if any.
     592            // Unfortunately it's possible for this to be called more than once for a node
     593            // because of the nature of mutation events (if the node is moved further in the child list
     594            // by an event handler).
     595            dispatchChildRemovalEvents(child.get());
     596            ChildFrameDisconnector(child.get()).disconnect();
     597
     598            // Mutation or unload events could have moved this child.
     599            if (child != m_firstChild)
     600                continue;
     601
     602            // We can't use a bulk version of document()->nodeWillBeRemoved() before the removal loop.
     603            // We need to call document()->nodeWillBeRemoved() on each node in case the node was created
     604            // by a mutation event handler.
     605            // document()->nodeWillbeRemoved() may modify the children list so we may need to retry.
     606            document()->nodeWillBeRemoved(child.get());
     607            if (child != m_firstChild)
     608                continue;
     609
     610            // Notify the mutation observers.
     611            mutation.willRemoveChild(child.get());
     612            child->notifyMutationObserversNodeWillDetach();
     613
     614            removeBetween(0, child->nextSibling(), child.get());
     615            removedChildren.append(child.release());
    643616        }
    644617
     618        // FIXME: We could avoid walking all the children twice by calling
     619        // notify inside the loop and childrenChanged after but that would mean
     620        // calling childrenChanged in a different order than all other methods.
     621        // Figure out if this is safe.
    645622        childrenChanged(false, 0, 0, -static_cast<int>(removedChildren.size()));
    646        
    647623        for (size_t i = 0; i < removedChildren.size(); ++i)
    648624            ChildNodeRemovalNotifier(this).notify(removedChildren[i].get());

  • trunk/Source/WebCore/dom/Document.cpp

    r147920 r147942  
    35693569}
    35703570
    3571 void Document::nodeChildrenWillBeRemoved(ContainerNode* container)
    3572 {
    3573     if (!m_ranges.isEmpty()) {
    3574         HashSet<Range*>::const_iterator end = m_ranges.end();
    3575         for (HashSet<Range*>::const_iterator it = m_ranges.begin(); it != end; ++it)
    3576             (*it)->nodeChildrenWillBeRemoved(container);
    3577     }
    3578 
    3579     HashSet<NodeIterator*>::const_iterator nodeIteratorsEnd = m_nodeIterators.end();
    3580     for (HashSet<NodeIterator*>::const_iterator it = m_nodeIterators.begin(); it != nodeIteratorsEnd; ++it) {
    3581         for (Node* n = container->firstChild(); n; n = n->nextSibling())
    3582             (*it)->nodeWillBeRemoved(n);
    3583     }
    3584 
    3585     if (Frame* frame = this->frame()) {
    3586         for (Node* n = container->firstChild(); n; n = n->nextSibling()) {
    3587             frame->eventHandler()->nodeWillBeRemoved(n);
    3588             frame->selection()->nodeWillBeRemoved(n);
    3589             frame->page()->dragCaretController()->nodeWillBeRemoved(n);
    3590         }
    3591     }
    3592 }
    3593 
    35943571void Document::nodeWillBeRemoved(Node* n)
    35953572{

  • trunk/Source/WebCore/dom/Document.h

    r147920 r147942  
    725725
    726726    void updateRangesAfterChildrenChanged(ContainerNode*);
    727     // nodeChildrenWillBeRemoved is used when removing all node children at once.
    728     void nodeChildrenWillBeRemoved(ContainerNode*);
    729727    // nodeWillBeRemoved is only safe when removing one node at a time.
    730728    void nodeWillBeRemoved(Node*);

  • trunk/Source/WebCore/dom/Range.cpp

    r147832 r147942  
    17331733}
    17341734
    1735 static inline void boundaryNodeChildrenWillBeRemoved(RangeBoundaryPoint& boundary, ContainerNode* container)
    1736 {
    1737     for (Node* nodeToBeRemoved = container->firstChild(); nodeToBeRemoved; nodeToBeRemoved = nodeToBeRemoved->nextSibling()) {
    1738         if (boundary.childBefore() == nodeToBeRemoved) {
    1739             boundary.setToStartOfNode(container);
    1740             return;
    1741         }
    1742 
    1743         for (Node* n = boundary.container(); n; n = n->parentNode()) {
    1744             if (n == nodeToBeRemoved) {
    1745                 boundary.setToStartOfNode(container);
    1746                 return;
    1747             }
    1748         }
    1749     }
    1750 }
    1751 
    1752 void Range::nodeChildrenWillBeRemoved(ContainerNode* container)
    1753 {
    1754     ASSERT(container);
    1755     ASSERT(container->document() == m_ownerDocument);
    1756     boundaryNodeChildrenWillBeRemoved(m_start, container);
    1757     boundaryNodeChildrenWillBeRemoved(m_end, container);
    1758 }
    1759 
    17601735static inline void boundaryNodeWillBeRemoved(RangeBoundaryPoint& boundary, Node* nodeToBeRemoved)
    17611736{

  • trunk/Source/WebCore/dom/Range.h

    r142461 r147942  
    129129
    130130    void nodeChildrenChanged(ContainerNode*);
    131     void nodeChildrenWillBeRemoved(ContainerNode*);
    132131    void nodeWillBeRemoved(Node*);
    133132
Note: See TracChangeset for help on using the changeset viewer.