Bug 130622 – [iOS WebKit2] Dictation support

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

Tracks the work required to support dictation in WK2 on iOS.

<rdar://problem/15337316>

[Enrica Casucci]

Created attachment 227504 [details]
Patch

[Benjamin Poulain]

Comment on attachment 227504 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Adding support for dictation on iOS. There are two main types of interaction

interaction(s)?

> Source/WebKit2/UIProcess/AutoCorrectionCallback.h:141
> +        ASSERT(m_callback);
> +
> +        m_callback(false, returnValue1, returnValue2, returnValue3);
> +

I think you can remove the blank line. This code is clear without paragraphs.

> Source/WebKit2/UIProcess/AutoCorrectionCallback.h:142
> +        m_callback = 0;

= nullptr;

> Source/WebKit2/UIProcess/AutoCorrectionCallback.h:151
> +        m_callback = 0;

= nullptr;

> Source/WebKit2/UIProcess/AutoCorrectionCallback.h:162
> +    };

Indent issue here.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:106
> +    UIWKDictationContextHandler _dictationHandler;

I don't understand why is this an attribute of the view interaction. Shouldn't it be just local to requestDictationContext:?

If it needs to be here, shouldn't it uses RetainPtr?

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1270
> +- (void)requestDictationContext:(void (^)(NSString *selected, NSString *prefix, NSString *postfix))completionHandler

Not sure if prefix/suffix is the best description for the variables here.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1273
> +    _page->requestDictationContext(DictationContextCallback::create([self](bool /*error*/, const String& selectedText, const String& beforeText, const String& afterText) {

Shouldn't you capture the dictationHandler instead of self?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1215
> +    if (startPosition != startOfEditableContent(startPosition)) {
> +        VisiblePosition currentPosition = startPosition;
> +        VisiblePosition lastPosition = startPosition;
> +        for (unsigned i = 0; i < dictationContextWordCount; ++i) {
> +            currentPosition = startOfWord(positionOfNextBoundaryOfGranularity(lastPosition, WordGranularity, DirectionBackward));
> +            if (currentPosition.isNull())
> +                break;
> +            lastPosition = currentPosition;
> +        }
> +        if (lastPosition.isNotNull() && lastPosition != startPosition)
> +            contextBefore = plainText(Range::create(*frame.document(), lastPosition, startPosition).get());
> +    }

Could you make this into a function taking the direction and wordCount as argument? The before and after cases look very similar.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1234
> +void WebPage::replaceDictatedText(const String& oldText, const String& newText)

If find this a little confusing. The name does not really match with the code.

oldText is unused, only its length is used here. The code does not seem related to dictation, just with the selection.

Shouldn't this be: replaceCharactersBeforeSelection(unsigned positionCount, const String& replacementText)?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1236
> +    RefPtr<Range> range;

This should be moved bellow where the range is used.

[Enrica Casucci]

Comment on attachment 227504 [details]
Patch

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

>> Source/WebKit2/UIProcess/AutoCorrectionCallback.h:151
>> +        m_callback = 0;
> 
> = nullptr;

Just copy pasted from other class that uses 0. I'll change it.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:106
>> +    UIWKDictationContextHandler _dictationHandler;
> 
> I don't understand why is this an attribute of the view interaction. Shouldn't it be just local to requestDictationContext:?
> 
> If it needs to be here, shouldn't it uses RetainPtr?

It doesn't need to be here.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1273
>> +    _page->requestDictationContext(DictationContextCallback::create([self](bool /*error*/, const String& selectedText, const String& beforeText, const String& afterText) {
> 
> Shouldn't you capture the dictationHandler instead of self?

Self refers to the callback, not the handler.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1234
>> +void WebPage::replaceDictatedText(const String& oldText, const String& newText)
> 
> If find this a little confusing. The name does not really match with the code.
> 
> oldText is unused, only its length is used here. The code does not seem related to dictation, just with the selection.
> 
> Shouldn't this be: replaceCharactersBeforeSelection(unsigned positionCount, const String& replacementText)?

Actually it should also check that the text matches. I want to leave the name that mentions dictation because in the future this code will be used to animate the dictated text.

[Enrica Casucci]

Created attachment 227690 [details]
Patch2

[Enrica Casucci]

Committed revision 166208.