Changeset 84265 in webkit


Ignore:
Timestamp:
Apr 19, 2011 11:05:52 AM (13 years ago)
Author:
rniwa@webkit.org
Message:

2011-04-19 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Dimitri Glazkov.

REGRESSION(r74228-75294): removing nodes is 200+ times slower when selection is inside a shadow DOM
​https://bugs.webkit.org/show_bug.cgi?id=57061

The bug was caused by Range::compareNode's incorrectly returning NODE_INSIDE when the selection is inside
a shadow DOM and the node is outside of the shadow DOM. This caused respondToNodeModification to call
RenderView::clearSelection every time a node is removed when selection is in a shadow DOM and resulted in
a significant performance regression.

Fixed Ranged::compareNode by making Range::compareBoundaryPoints throw a WRONG_DOCUMENT_ERR when there are
no common ancestors between containerA and containerB. This will force compareNode to also throw an exception
and prevents respondToNodeModification from clearing selection.

No new tests because this is a performance improvement and the fix in Range cannot be tested since shadow DOM
isn't exposed to JavaScript.

  • dom/Range.cpp: (WebCore::Range::setStart): Calls compareBoundaryPoints; since we ensures that the root container noes of start and end nodes are same, we should never get an exception from compareBoundaryPoints. (WebCore::Range::setEnd): Ditto. (WebCore::Range::isPointInRange): Calls compareBoundaryPoints; returns false when compareBoundaryPoints throws an exception. (WebCore::Range::comparePoint): Calls compareBoundaryPoints; exit early when an exception is thrown by compareBoundaryPoints. (WebCore::Range::compareBoundaryPoints): Throws an exception when two containers do not have a common ancestor. (WebCore::Range::boundaryPointsValid): Calls compareBoundaryPoints and checks that it didn't throw an exception.
  • dom/Range.h:
  • editing/SelectionController.cpp: (WebCore::SelectionController::respondToNodeModification):
  • editing/htmlediting.cpp: (WebCore::comparePositions): Calls compareBoundaryPoints.
  • editing/markup.cpp: (WebCore::createMarkup): Calls compareBoundaryPoints; since startNode and pastEnd are both in the same document and neither are in a shadow DOM, it should never throw an exception.
  • page/DOMSelection.cpp: (WebCore::DOMSelection::containsNode): Calls compareBoundaryPoints; node is fully selected only if no exception was thrown.
Location:
trunk/Source/WebCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r84264 r84265  
     12011-04-19  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Reviewed by Dimitri Glazkov.
     4
     5        REGRESSION(r74228-75294): removing nodes is 200+ times slower when selection is inside a shadow DOM
     6        https://bugs.webkit.org/show_bug.cgi?id=57061
     7
     8        The bug was caused by Range::compareNode's incorrectly returning NODE_INSIDE when the selection is inside
     9        a shadow DOM and the node is outside of the shadow DOM. This caused respondToNodeModification to call
     10        RenderView::clearSelection every time a node is removed when selection is in a shadow DOM and resulted in
     11        a significant performance regression.
     12
     13        Fixed Ranged::compareNode by making Range::compareBoundaryPoints throw a WRONG_DOCUMENT_ERR when there are
     14        no common ancestors between containerA and containerB. This will force compareNode to also throw an exception
     15        and prevents respondToNodeModification from clearing selection.
     16
     17        No new tests because this is a performance improvement and the fix in Range cannot be tested since shadow DOM
     18        isn't exposed to JavaScript.
     19
     20        * dom/Range.cpp:
     21        (WebCore::Range::setStart): Calls compareBoundaryPoints; since we ensures that the root container noes of
     22        start and end nodes are same, we should never get an exception from compareBoundaryPoints.
     23        (WebCore::Range::setEnd): Ditto.
     24        (WebCore::Range::isPointInRange): Calls compareBoundaryPoints; returns false when compareBoundaryPoints
     25        throws an exception.
     26        (WebCore::Range::comparePoint): Calls compareBoundaryPoints; exit early when an exception is thrown by
     27        compareBoundaryPoints.
     28        (WebCore::Range::compareBoundaryPoints): Throws an exception when two containers do not have a common ancestor.
     29        (WebCore::Range::boundaryPointsValid): Calls compareBoundaryPoints and checks that it didn't throw an exception.
     30        * dom/Range.h:
     31        * editing/SelectionController.cpp:
     32        (WebCore::SelectionController::respondToNodeModification):
     33        * editing/htmlediting.cpp:
     34        (WebCore::comparePositions): Calls compareBoundaryPoints.
     35        * editing/markup.cpp:
     36        (WebCore::createMarkup): Calls compareBoundaryPoints; since startNode and pastEnd are both in the same document
     37        and neither are in a shadow DOM, it should never throw an exception.
     38        * page/DOMSelection.cpp:
     39        (WebCore::DOMSelection::containsNode): Calls compareBoundaryPoints; node is fully selected only if no exception
     40        was thrown.
     41
    1422011-04-19  Jungshik Shin  <jshin@chromium.org>
    243

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

    r82778 r84265  
    225225        collapse(true, ec);
    226226    // check if new start after end
    227     else if (compareBoundaryPoints(m_start, m_end) > 0)
     227    else if (compareBoundaryPoints(m_start, m_end, ec) > 0) {
     228        ASSERT(!ec);
    228229        collapse(true, ec);
     230    }
    229231}
    230232
    … …  
    263265        collapse(false, ec);
    264266    // check if new end before start
    265     if (compareBoundaryPoints(m_start, m_end) > 0)
     267    if (compareBoundaryPoints(m_start, m_end, ec) > 0) {
     268        ASSERT(!ec);
    266269        collapse(false, ec);
     270    }
    267271}
    268272
    … …  
    307311        return false;
    308312
    309     return compareBoundaryPoints(refNode, offset, m_start.container(), m_start.offset()) >= 0
    310         && compareBoundaryPoints(refNode, offset, m_end.container(), m_end.offset()) <= 0;
     313    return compareBoundaryPoints(refNode, offset, m_start.container(), m_start.offset(), ec) >= 0 && !ec
     314        && compareBoundaryPoints(refNode, offset, m_end.container(), m_end.offset(), ec) <= 0 && !ec;
    311315}
    312316
    … …  
    338342
    339343    // compare to start, and point comes before
    340     if (compareBoundaryPoints(refNode, offset, m_start.container(), m_start.offset()) < 0)
     344    if (compareBoundaryPoints(refNode, offset, m_start.container(), m_start.offset(), ec) < 0)
    341345        return -1;
    342346
     347    if (ec)
     348        return 0;
     349
    343350    // compare to end, and point comes after
    344     if (compareBoundaryPoints(refNode, offset, m_end.container(), m_end.offset()) > 0)
     351    if (compareBoundaryPoints(refNode, offset, m_end.container(), m_end.offset(), ec) > 0 && !ec)
    345352        return 1;
    346353
    … …  
    434441    switch (how) {
    435442        case START_TO_START:
    436             return compareBoundaryPoints(m_start, sourceRange->m_start);
     443            return compareBoundaryPoints(m_start, sourceRange->m_start, ec);
    437444        case START_TO_END:
    438             return compareBoundaryPoints(m_end, sourceRange->m_start);
     445            return compareBoundaryPoints(m_end, sourceRange->m_start, ec);
    439446        case END_TO_END:
    440             return compareBoundaryPoints(m_end, sourceRange->m_end);
     447            return compareBoundaryPoints(m_end, sourceRange->m_end, ec);
    441448        case END_TO_START:
    442             return compareBoundaryPoints(m_start, sourceRange->m_end);
     449            return compareBoundaryPoints(m_start, sourceRange->m_end, ec);
    443450    }
    444451
    … …  
    447454}
    448455
    449 short Range::compareBoundaryPoints(Node* containerA, int offsetA, Node* containerB, int offsetB)
     456short Range::compareBoundaryPoints(Node* containerA, int offsetA, Node* containerB, int offsetB, ExceptionCode& ec)
    450457{
    451458    ASSERT(containerA);
    … …  
    508515    // ### we need to do a traversal here instead
    509516    Node* commonAncestor = commonAncestorContainer(containerA, containerB);
    510     if (!commonAncestor)
    511         return 0;
     517    if (!commonAncestor) {
     518        ec = WRONG_DOCUMENT_ERR;
     519        return 0;
     520    }
    512521    Node* childA = containerA;
    513522    while (childA && childA->parentNode() != commonAncestor)
    … …  
    538547}
    539548
    540 short Range::compareBoundaryPoints(const RangeBoundaryPoint& boundaryA, const RangeBoundaryPoint& boundaryB)
    541 {
    542     return compareBoundaryPoints(boundaryA.container(), boundaryA.offset(), boundaryB.container(), boundaryB.offset());
     549short Range::compareBoundaryPoints(const RangeBoundaryPoint& boundaryA, const RangeBoundaryPoint& boundaryB, ExceptionCode& ec)
     550{
     551    return compareBoundaryPoints(boundaryA.container(), boundaryA.offset(), boundaryB.container(), boundaryB.offset(), ec);
    543552}
    544553
    545554bool Range::boundaryPointsValid() const
    546555{
    547     return m_start.container() && compareBoundaryPoints(m_start, m_end) <= 0;
     556    ExceptionCode ec = 0;
     557    return m_start.container() && compareBoundaryPoints(m_start, m_end, ec) <= 0 && !ec;
    548558}
    549559

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

    r82533 r84265  
    7070    enum CompareHow { START_TO_START, START_TO_END, END_TO_END, END_TO_START };
    7171    short compareBoundaryPoints(CompareHow, const Range* sourceRange, ExceptionCode&) const;
    72     static short compareBoundaryPoints(Node* containerA, int offsetA, Node* containerB, int offsetB);
    73     static short compareBoundaryPoints(const RangeBoundaryPoint& boundaryA, const RangeBoundaryPoint& boundaryB);
     72    static short compareBoundaryPoints(Node* containerA, int offsetA, Node* containerB, int offsetB, ExceptionCode&);
     73    static short compareBoundaryPoints(const RangeBoundaryPoint& boundaryA, const RangeBoundaryPoint& boundaryB, ExceptionCode&);
    7474    bool boundaryPointsValid() const;
    7575    bool intersectsNode(Node* refNode, ExceptionCode&);

  • trunk/Source/WebCore/editing/SelectionController.cpp

    r82593 r84265  
    252252        else
    253253            m_selection.setWithoutValidation(m_selection.end(), m_selection.start());
    254     } else if (m_selection.firstRange()) {
     254    } else if (RefPtr<Range> range = m_selection.firstRange()) {
    255255        ExceptionCode ec = 0;
    256         Range::CompareResults compareResult = m_selection.firstRange()->compareNode(node, ec);
     256        Range::CompareResults compareResult = range->compareNode(node, ec);
    257257        if (!ec && (compareResult == Range::NODE_BEFORE_AND_AFTER || compareResult == Range::NODE_INSIDE)) {
    258258            // If we did nothing here, when this node's renderer was destroyed, the rect that it

  • trunk/Source/WebCore/editing/htmlediting.cpp

    r83322 r84265  
    123123    }
    124124
    125     int result = Range::compareBoundaryPoints(nodeA, offsetA, nodeB, offsetB);
     125    ExceptionCode ec;
     126    int result = Range::compareBoundaryPoints(nodeA, offsetA, nodeB, offsetB, ec);
    126127    return result ? result : bias;
    127128}

  • trunk/Source/WebCore/editing/markup.cpp

    r81295 r84265  
    572572        startNode = visibleStart.next().deepEquivalent().deprecatedNode();
    573573
    574         if (pastEnd && Range::compareBoundaryPoints(startNode, 0, pastEnd, 0) >= 0) {
     574        ExceptionCode ec = 0;
     575        if (pastEnd && Range::compareBoundaryPoints(startNode, 0, pastEnd, 0, ec) >= 0) {
     576            ASSERT(!ec);
    575577            if (deleteButton)
    576578                deleteButton->enable();
    577579            return interchangeNewlineString;
    578580        }
     581        ASSERT(!ec);
    579582    }
    580583

  • trunk/Source/WebCore/page/DOMSelection.cpp

    r82588 r84265  
    475475
    476476    ExceptionCode ec = 0;
    477     bool nodeFullySelected = Range::compareBoundaryPoints(parentNode, nodeIndex, selectedRange->startContainer(ec), selectedRange->startOffset(ec)) >= 0
    478         && Range::compareBoundaryPoints(parentNode, nodeIndex + 1, selectedRange->endContainer(ec), selectedRange->endOffset(ec)) <= 0;
     477    bool nodeFullySelected = Range::compareBoundaryPoints(parentNode, nodeIndex, selectedRange->startContainer(ec), selectedRange->startOffset(ec), ec) >= 0 && !ec
     478        && Range::compareBoundaryPoints(parentNode, nodeIndex + 1, selectedRange->endContainer(ec), selectedRange->endOffset(ec), ec) <= 0 && !ec;
    479479    ASSERT(!ec);
    480480    if (nodeFullySelected)
    481481        return true;
    482482
    483     bool nodeFullyUnselected = Range::compareBoundaryPoints(parentNode, nodeIndex, selectedRange->endContainer(ec), selectedRange->endOffset(ec)) > 0
    484         || Range::compareBoundaryPoints(parentNode, nodeIndex + 1, selectedRange->startContainer(ec), selectedRange->startOffset(ec)) < 0;
     483    bool nodeFullyUnselected = (Range::compareBoundaryPoints(parentNode, nodeIndex, selectedRange->endContainer(ec), selectedRange->endOffset(ec), ec) > 0 && !ec)
     484        || (Range::compareBoundaryPoints(parentNode, nodeIndex + 1, selectedRange->startContainer(ec), selectedRange->startOffset(ec), ec) < 0 && !ec);
    485485    ASSERT(!ec);
    486486    if (nodeFullyUnselected)
Note: See TracChangeset for help on using the changeset viewer.