[webkit-reviews] review denied: [Bug 130622] [iOS WebKit2] Dictation support : [Attachment 227504] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 130622: [iOS WebKit2] Dictation support
https://bugs.webkit.org/show_bug.cgi?id=130622

Attachment 227504: Patch
https://bugs.webkit.org/attachment.cgi?id=227504&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.