Bug 131827 – [iOS WebKit2] support replacements for misspelled words

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

This tracks the work required to support replacements on iOS for WebKit2.

<rdar://problem/16319657>

[Enrica Casucci]

Created attachment 229593 [details]
Patch

[Benjamin Poulain]

Comment on attachment 229593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229593&action=review

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:136
> +@interface UITextInteractionAssistant (Staging)

Staging -> Private if you don't plan to expose it in UIKit.

[Enrica Casucci]

Committed revision 167469. This is part one.

[Enrica Casucci]

Created attachment 229695 [details]
Patch - part2

[WebKit Commit Bot]

Attachment 229695 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:697:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1340:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/editing/VisibleUnits.h:107:  The parameter name "position" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/editing/VisibleUnits.cpp:1819:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Enrica Casucci]

Created attachment 229698 [details]
Updated part 2

Fixed silly mistakes.

[Darin Adler]

Comment on attachment 229698 [details]
Updated part 2

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
> [phone]  Enrica Casucci  <[e-mail]>
> +
> +        [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.

[Enrica Casucci]

> > 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?
> 
This is a mistake. I want to convert the nbsp to space. I originally had an if statement and made a mistake when cleaning up the code. I'll fix this. Thanks for catching it!


> > Source/WebKit2/ChangeLog:70
> > +<<<<<<< .mine
> > [phone]  Enrica Casucci  <[e-mail]>
> > +
> > +        [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.

[Enrica Casucci]

Committed revision 167624.