Bug 130622 - [iOS WebKit2] Dictation support
Summary: [iOS WebKit2] Dictation support
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-03-21 15:57 PDT by Enrica Casucci
Modified: 2014-03-24 16:52 PDT (History)
0 users

See Also:


Attachments
Patch (16.28 KB, patch)
2014-03-21 16:02 PDT, Enrica Casucci
benjamin: review-
Details | Formatted Diff | Diff
Patch2 (15.92 KB, patch)
2014-03-24 13:52 PDT, Enrica Casucci
benjamin: 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-03-21 15:57:24 PDT
Tracks the work required to support dictation in WK2 on iOS.

<rdar://problem/15337316>
Comment 1 Enrica Casucci 2014-03-21 16:02:52 PDT
Created attachment 227504 [details]
Patch
Comment 2 Benjamin Poulain 2014-03-24 12:37:52 PDT
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.
Comment 3 Enrica Casucci 2014-03-24 13:27:43 PDT
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.
Comment 4 Enrica Casucci 2014-03-24 13:52:02 PDT
Created attachment 227690 [details]
Patch2
Comment 5 Enrica Casucci 2014-03-24 16:52:44 PDT
Committed revision 166208.