[webkit-reviews] review granted: [Bug 131827] [iOS WebKit2] support replacements for misspelled words : [Attachment 229698] Updated part 2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 131827: [iOS WebKit2] support replacements for misspelled words
https://bugs.webkit.org/show_bug.cgi?id=131827

Attachment 229698: Updated part 2
https://bugs.webkit.org/attachment.cgi?id=229698&action=review

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


> Source/WebCore/editing/VisibleUnits.cpp:1825
> +    String characterString =
plainText(Range::create(position.deepEquivalent().anchorNode()->document(),
previousPosition, position).get(), TextIteratorDefaultBehavior, true);
> +    if (characterString.isEmpty())
> +	   return 0;

Creating a string for these two characters seems a excessive; all the overhead
of using a string builder inside the plainText function has a significant cost.
If performance is an issue at all, we can make a much faster version that
doesn’t use the plainText function.

> Source/WebCore/editing/VisibleUnits.cpp:1833
> +    const UChar32 NonBreakingSpaceCharacter = 0xA0;

CharacterNames.h already has a constant for this, noBreakSpace, which I suggest
we use instead of defining a new one. The name “NO BREAK SPACE” comes right out
of the Unicode documentation.

> Source/WebCore/editing/VisibleUnits.cpp:1834
> +    return characterString[0] ? NonBreakingSpaceCharacter : ' ';

I don’t understand the logic here. The null character turns into non-breaking
space, the non-null character turns into breaking space. Don’t we sometimes
want to return the character itself?

> Source/WebKit2/ChangeLog:70
> +<<<<<<< .mine
> +2014-04-18  Enrica Casucci  <enrica at apple.com>
> +
> +	   [iOS WebKit2] support replacements for misspelled words.
> +	   https://bugs.webkit.org/show_bug.cgi?id=131827
> +	   <rdar://problem/16319657>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   This is the second a final piece to support replacements.
> +	   It adds some entrypoints used by the keyboard code to perform
> +	   replacement when reaching the edge of a word using backspace.
> +	   I've modified the behavior of replaceSelectedText to work both
> +	   with caret or range selections.
> +
> +	   * Shared/EditorState.cpp:
> +	   (WebKit::EditorState::encode):
> +	   (WebKit::EditorState::decode):
> +	   * Shared/EditorState.h:
> +	   (WebKit::EditorState::EditorState):
> +	   * UIProcess/ios/WKContentViewInteraction.mm:
> +	   (-[WKContentView isReplaceAllowed]):
> +	   (-[WKContentView _characterBeforeCaretSelection]):
> +	   * WebProcess/WebPage/WebPage.cpp:
> +	   (WebKit::WebPage::editorState):
> +	   * WebProcess/WebPage/ios/WebPageIOS.mm:
> +	   (WebKit::WebPage::replaceSelectedText):
> +
> +=======

Looks like this accidentally includes a second change log.