[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


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #80458|review?                     |review-
               Flag|                            |




--- Comment #5 from mitz at webkit.org  2011-01-31 18:16:33 PST ---
(From update of attachment 80458)
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/ChangeLog:13
> +           is achieved by explicitly apply pending correction when user types space, line break or

explicitly applying?

> Source/WebCore/ChangeLog:43
> +        (WebCore::markerTypesForAutocorrection): Add SpellCheckingExemption marker when apply correction.

applying!

> Source/WebCore/ChangeLog:53
> +        (WebCore::Editor::markAllMisspellingsAndBadGrammarInRanges): Don't marker mispelling if the

typo: marker -> mark

> Source/WebCore/dom/DocumentMarker.h:44
> +        // correction. Text with Replacement marker don't necessarilly has CorrectionIndicator

typo: don't -> doesn't, necessarilly -> necessarily

> Source/WebCore/dom/DocumentMarker.h:45
> +        // marker. For instance, after some text has been corrected, it will has both Replacement

has->have

> 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.

> Source/WebCore/editing/Editor.cpp:2678
> +    document->markers()->removeMarkers(wordRange.get(), DocumentMarker::Spelling | DocumentMarker::CorrectionIndicator | DocumentMarker::SpellCheckingExemption, true);

This mysterious “true” at the call site suggests that the parameter should be an enum.

> Source/WebCore/editing/Editor.cpp:2736
> +    // Only apply correction if current correction panel type is PanelTypeCorrection.

This comment is redundant with the following code.

> Source/WebCore/editing/Editor.cpp:2740
> +    // Only apply correction if the caret is at the end of the range to be replaced.

Even this comment doesn’t add much. Perhaps you could explain the motivation instead.

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