Bug 131827 - [iOS WebKit2] support replacements for misspelled words
Summary: [iOS WebKit2] support replacements for misspelled words
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-17 16:13 PDT by Enrica Casucci
Modified: 2014-04-21 14:15 PDT (History)
1 user (show)

See Also:


Attachments
Patch (14.06 KB, patch)
2014-04-17 16:30 PDT, Enrica Casucci
benjamin: review+
Details | Formatted Diff | Diff
Patch - part2 (9.59 KB, patch)
2014-04-18 17:49 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Updated part 2 (11.61 KB, patch)
2014-04-18 18:26 PDT, Enrica Casucci
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2014-04-17 16:13:04 PDT
This tracks the work required to support replacements on iOS for WebKit2.

<rdar://problem/16319657>
Comment 1 Enrica Casucci 2014-04-17 16:30:24 PDT
Created attachment 229593 [details]
Patch
Comment 2 Benjamin Poulain 2014-04-17 16:36:16 PDT
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.
Comment 3 Enrica Casucci 2014-04-17 17:02:45 PDT
Committed revision 167469. This is part one.
Comment 4 Enrica Casucci 2014-04-18 17:49:14 PDT
Created attachment 229695 [details]
Patch - part2
Comment 5 WebKit Commit Bot 2014-04-18 17:51:28 PDT
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.
Comment 6 Enrica Casucci 2014-04-18 18:26:14 PDT
Created attachment 229698 [details]
Updated part 2

Fixed silly mistakes.
Comment 7 Darin Adler 2014-04-19 14:46:23 PDT
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
> +2014-04-18  Enrica Casucci  <enrica@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.
Comment 8 Enrica Casucci 2014-04-21 10:52:59 PDT
> > 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
> > +2014-04-18  Enrica Casucci  <enrica@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.
Comment 9 Enrica Casucci 2014-04-21 14:15:26 PDT
Committed revision 167624.