[webkit-reviews] review granted: [Bug 209308] Have insertDictatedTextAsync() take an InsertTextOptions : [Attachment 394025] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 209308: Have insertDictatedTextAsync() take an InsertTextOptions
https://bugs.webkit.org/show_bug.cgi?id=209308

Attachment 394025: Patch

https://bugs.webkit.org/attachment.cgi?id=394025&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 394025
  --> https://bugs.webkit.org/attachment.cgi?id=394025
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:4850
> +	   InsertTextOptions options;
> +	   options.registerUndoGroup = registerUndoGroup;
> +	   m_page->insertDictatedTextAsync(eventText, replacementRange,
dictationAlternatives, WTFMove(options));

This will be nicer when we get C++ 20’s designated initializers so we can just
write "InsertTextOptions { .registerUndoGroup = registerUndoGroup }" without
WTFMove or a local variable.

> Source/WebKit/UIProcess/WebPageProxy.h:858
> +    void insertDictatedTextAsync(const String&, const EditingRange&
replacementRange, const Vector<WebCore::TextAlternativeWithRange>&,
InsertTextOptions&&);

I think it’s a peculiar choice to use move semantics for a structure that just
contains a few bits of scalar data. This would have value if the options were
more complex and so maybe this is good planning for the future.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4721
> +	   _page->insertDictatedTextAsync(aStringValue, WebKit::EditingRange {
}, { textAlternativeWithRange }, WebKit::InsertTextOptions { });

Do we really have to write out the type names here? I don’t think they add
much.