[Webkit-unassigned] [Bug 53255] Reversion should not be marked as misspelled.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53255





--- Comment #6 from jpu at apple.com  2011-02-01 09:19:34 PST ---
(In reply to comment #5)
> (From update of attachment 80458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80458&action=review
> 
> I am going to say r- because of the hasMarkers() change. I don’t understand why it’s needed and it can certainly be dealt with separately from this bug.
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:591
> > -    Node* startContainer = range->startContainer();
> > -    ASSERT(startContainer);
> > -    Node* endContainer = range->endContainer();
> > -    ASSERT(endContainer);
> > -
> > -    Node* pastLastNode = range->pastLastNode();
> > -    for (Node* node = range->firstNode(); node != pastLastNode; node = node->traverseNextNode()) {
> > +    TextIterator it(range);
> > +
> > +    // Need special handling on first and last node.
> > +    RefPtr<Range> firstRange = it.range();
> > +    Node* firstNode = firstRange->startContainer();
> > +    RefPtr<Range> previousRange = firstRange;
> > +    it.advance();
> > +    for (; !it.atEnd(); it.advance()) {
> > +        Node* node = previousRange->startContainer();
> > +        // In this loop, we check every node between (excluding) the first and last node.
> > +        if (node == firstNode)
> > +            continue;
> >          Vector<DocumentMarker> markers = markersForNode(node);
> > -        Vector<DocumentMarker>::const_iterator end = markers.end();
> > -        for (Vector<DocumentMarker>::const_iterator it = markers.begin(); it != end; ++it) {
> > -            if (!(markerTypes & it->type))
> > -                continue;
> > -            if (node == startContainer && node == endContainer) {
> > -                // The range spans only one node.
> > -                if (it->endOffset > static_cast<unsigned>(range->startOffset()) && it->startOffset < static_cast<unsigned>(range->endOffset()))
> > -                    return true;
> > -            } else {
> > -                if (node == startContainer) {
> > -                    if (it->endOffset > static_cast<unsigned>(range->startOffset()))
> > -                        return true;
> > -                } else if (node == endContainer) {
> > -                    if (it->startOffset < static_cast<unsigned>(range->endOffset()))
> > -                        return true;
> > -                } else
> > -                    return true;
> > -            }
> > +        size_t size = markers.size();
> > +        for (size_t i = 0; i < size; ++i) {
> > +            const DocumentMarker& marker = markers[i];
> > +            if (marker.type & markerTypes)
> > +                return true;
> > +        }
> > +        previousRange = it.range();
> > +    }
> > +
> > +    RefPtr<Range> lastRange = previousRange;
> > +    Node* lastNode = lastRange->startContainer();
> > +
> > +    // Check the first node.
> > +    Vector<DocumentMarker> markers = markersForNode(firstNode);
> > +    size_t size = markers.size();
> > +    for (size_t i = 0; i < size; ++i) {
> > +        const DocumentMarker& marker = markers[i];
> > +        if (!(marker.type & markerTypes))
> > +            continue;
> > +        if (firstNode == lastNode) {
> > +            if (marker.endOffset > static_cast<unsigned>(firstRange->startOffset()) && marker.startOffset < static_cast<unsigned>(lastRange->endOffset()))
> > +                return true;
> > +        } else {
> > +            if (marker.endOffset > static_cast<unsigned>(firstRange->startOffset()))
> > +                return true;
> >          }
> >      }
> > +
> > +    if (firstNode == lastNode)
> > +        return false;
> > +    // Didn't find relevant mark in all other nodes. Now check the last node.
> > +    markers = markersForNode(lastNode);
> > +    size = markers.size();
> > +    for (size_t i = 0; i < size; ++i) {
> > +        const DocumentMarker& marker = markers[i];
> > +        if (!(marker.type & markerTypes))
> > +            continue;
> > +        if (marker.startOffset < static_cast<unsigned>(lastRange->endOffset()))
> > +            return true;
> > +    }
> > +
> 
> The new code is more complex and harder to follow than the old code. I also don’t think that a TextIterator is more efficient than node traversal.

The concern here is mainly about correctness, rather than efficiency. My understanding is that TextIterator guarantees to iterate on leave text nodes, while regular node iteration doesn't decsend into children node. Seeing how addMarker() is implemented, I thought using TextIterator is the correct thing to do. If this change is entirely unnecessary, I could pull out the change on that function. If motivation is plausible, I will try to restructure the code for better readability.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.