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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 93000: Patch (v2)
https://bugs.webkit.org/attachment.cgi?id=93000&action=review

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

> Source/WebCore/dom/DocumentMarker.h:57
> +	   // This marker indicate user has deleted an autocorrection starting
at the end of the

Should be “This marker indicates”.

> Source/WebCore/dom/DocumentMarker.h:58
> +	   // range that bears this marker. In some platforms, if user later
inserts the same original

Should be “if the user”.

> Source/WebCore/dom/DocumentMarker.h:60
> +	   // marker is the original word before autocorrection is applied.

Should be “was applied”.

> Source/WebCore/editing/CompositeEditCommand.cpp:344
> +void
CompositeEditCommand::replaceTextInNodeAndPreserveMarkers(PassRefPtr<Text>
prpNode, unsigned offset, unsigned count, const String& replacementText)

This is good. A name I like slightly better is
replaceTextInNodePreservingMarkers. I’m OK with your current name or with my
suggested name.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:760
> +    if (markers.isEmpty())
> +	   return String();

This special case should be removed. I suggested that in the last review at a
different call site but somehow missed it here.

> Source/WebCore/editing/visible_units.cpp:316
> +bool isStartOfWord(const VisiblePosition &p)

The "&" should be one character to the left. The name of the local variable
should be “position”, not “p”.

> Source/WebCore/editing/visible_units.h:47
> +bool isStartOfWord(const VisiblePosition &);

There should not be a space before the "&".