[webkit-reviews] review denied: [Bug 53255] Reversion should not be marked as misspelled. : [Attachment 80458] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied jpu at apple.com's request for review:
Bug 53255: Reversion should not be marked as misspelled.
https://bugs.webkit.org/show_bug.cgi?id=53255

Attachment 80458: Patch
https://bugs.webkit.org/attachment.cgi?id=80458&action=review

------- Additional Comments from mitz at webkit.org
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.