[webkit-reviews] review denied: [Bug 60555] Autocorrection persists after deleting and retyping the same word at same location. : [Attachment 92968] Patch (v1)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jia Pu <jpu at apple.com>'s request for
review:
Bug 60555: Autocorrection persists after deleting and retyping the same word at
same location.
https://bugs.webkit.org/show_bug.cgi?id=60555

Attachment 92968: Patch (v1)
https://bugs.webkit.org/attachment.cgi?id=92968&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=92968&action=review

Looks good. Please consider my comments.

> Source/WebCore/editing/CompositeEditCommand.cpp:338
> +void CompositeEditCommand::replaceTextInNode(PassRefPtr<Text> node, unsigned
offset, unsigned count, const String& replacementText, bool preserveMarkers)

The preserveMarkers behavior completely wrap the old behavior and is not
intertwined with it. Given that, we should just add a new function, instead of
adding a boolean to the existing function to change its behavior. This will be
much clearer at callsites than a mystery "true".

> Source/WebCore/editing/CompositeEditCommand.cpp:341
> +    RefPtr<Text> textNode(node);

The names here are close enough that this is error prone. A more usual naming
scheme is to name the argument prpNode and the local variable node.

> Source/WebCore/editing/CompositeEditCommand.cpp:489
> +	   replaceTextInNode(textNode, upstream, length, rebalancedString,
true);

The meaning of true here is unclear. It would be better to have either two
separate functions, or use an enum for this sort of thing. I favor the two
separate functions.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:756
> +    VisiblePosition wordStart = startOfWord(startOfSelection,
RightWordIfOnBoundary);
> +    if (startOfSelection != wordStart)
> +	   return String();

I don’t think the local variable wordStart makes the code easy to read.

Also, it would be better to call a function to check that something is at a
start of a word rather than computing the start of the word and explicitly
checking !=.

For other units, like lines, we have isStartOfLine functions. I think we should
have that for words too.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:763
> +	   DocumentMarker marker = markers[i];

Copying into a local variable is unnecessarily inefficient. I suggest using a
const& instead.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:764
> +	   if ((int)marker.startOffset ==
startOfSelection.deepEquivalent().offsetInContainerNode())

The typecast here is undesirable. We’d be better off with a local variable and
an implicit conversion rather than a typecast. Also, we prefer C++ casts to
C-style casts.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:846
> +    Frame* frame = document()->frame();
> +    if (!originalString.isEmpty() && frame)
> +	   frame->editor()->deletedAutocorrectionAtPosition(m_endingPosition,
originalString);

Seems wasteful to fetch the frame even if originalString is empty.

> Source/WebCore/editing/DeleteSelectionCommand.h:76
> +    // On platforms that support autocorrection panel, if a deletion command
deletes
> +    // a correction, we want to keep track of original string replaced by
correction.
> +    // So that when user type the original string again, we won't apply the
same correction
> +    // again. This function provides access to original string after the
correction has
> +    // been deleted.
> +    String originalStringForAutocorrectionAtBeginningOfSelection();

Comment here seems a little too long and redundant with the comment for the
DeletedAutocorrection bit. Maybe we can tighten this up a bit.

> Source/WebCore/editing/Editor.cpp:2236
> +	       bool existingMarkersPermitReplacement =
m_spellingCorrector->processMarkersOnTextToBeReplacedByResult(result,
rangeToReplace.get(), replacedString);
>  
> -	       // Don't correct spelling in an already-corrected word.
> -	       DocumentMarkerController* markers =
m_frame->document()->markers();
> -	       if (markers->hasMarkers(rangeToReplace.get(),
DocumentMarker::Replacement)) {
> -		   doReplacement = false;
> -		   if (result->type == TextCheckingTypeCorrection)
> -		      
m_spellingCorrector->recordSpellcheckerResponseForModifiedCorrection(rangeToRep
lace.get(), replacedString, result->replacement);
> -	       } else if (markers->hasMarkers(rangeToReplace.get(),
DocumentMarker::RejectedCorrection))
> -		   doReplacement = false;
> -
> -	       if (!(shouldPerformReplacement || shouldShowCorrectionPanel) ||
!doReplacement)
> +	       if (!(shouldPerformReplacement || shouldShowCorrectionPanel) ||
!doReplacement || !existingMarkersPermitReplacement)
>		   continue;

Seems slightly wasteful to compute existingMarkersPermitReplacement even if the
other tests are true. I suggest you move the other early exists earlier in the
function and only compute replacedString and existingMarkersPermitReplacement
after those.

> Source/WebCore/editing/SpellingCorrectionController.cpp:526
> +    if (string.isEmpty() || !isWhitespace(string[string.length()-1]))

Should use spaces around the "-" here. Is the isWhitespace check the correct
type of whitespace check? I ask because there are many different whitespace
definitions used in different contexts.

> Source/WebCore/editing/SpellingCorrectionController.cpp:553
> +    if (markers.isEmpty())
> +	   return true;

No need for this early exit; the code already will efficiently do just this
without an explicit check.

> Source/WebCore/editing/SpellingCorrectionController.h:54
>  class TextCheckerClient;
> +struct TextCheckingResult;
>  class VisibleSelection;

The struct should have its own paragraph rather than being sorted in with the
classes.

> Source/WebCore/editing/SpellingCorrectionController.h:121
> +    // This function return false, if the replacement should not be carried
out.

No need for the comma here.