Bug 209308 – Have insertDictatedTextAsync() take an InsertTextOptions

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Have insertDictatedTextAsync() take an InsertTextOptions. Currently it takes a single bool as to whether to register an undo group and the implementation stack-allocates a InsertTextOptions and passes this state through, but it would provide future flexibility and make the signature of insertDictatedTextAsync() more closely look like insertTextAsync() if it took a InsertTextOptions that the caller passed.

My primary motivation for this change is to provide future flexibility as I plan to introduce a new insertion option that I want to affect dictated insertion.

[Radar WebKit Bug Importer]

<rdar://problem/60652838>

[Daniel Bates]

Created attachment 394025 [details]
Patch

[Darin Adler]

Comment on attachment 394025 [details]
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.

[Daniel Bates]

Comment on attachment 394025 [details]
Patch

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

Thanks for the review.

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:4850
>> +        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.

Yep, I know....I was tempted.

>> 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.

It's part future proofing + part me thinking in terms of whether this concept is about ownership transfer or not. InsertTextOptions is only ever used to make the call and the caller doesn't need to store it or re-purpose it. It's purely temporary. So, ownership transfer. I think that's the right model here. I am open to learning different perspectives..

>> 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.

I'm going to trust you on this one because I want to make you happy and I want to get this change in as I need it for my next step. If you ever have a moment can you please explain why passing consecutive { } arounds does NOT suffer from the same ambiguity as passing consecutive boolean arguments? In my opinion, it's the same problem and I grew up around passing enums instead of raw true or false because of it. Could also use named locals or inline comments too. The former hasn't grown on me enough to do without thought and the latter tends to be the thing I reach for as a last resort.

FYI, you can always pass { } for all POD types + default constructible.

void f(bool, bool, const WebKit::EditingRange&, WebKit::EditingRange&&);

Compiler will happily let you call it f({ }, { }, { }, { }) <-- I don't know what is being passed. This is why I like to write out the types when there are consecutive default constructible things and why I don't like to initialize PODs using just { } <-- empty inside.

I would ❤️ to hear your thoughts.

[Daniel Bates]

Comment on attachment 394025 [details]
Patch

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

>>> 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.
> 
> I'm going to trust you on this one because I want to make you happy and I want to get this change in as I need it for my next step. If you ever have a moment can you please explain why passing consecutive { } arounds does NOT suffer from the same ambiguity as passing consecutive boolean arguments? In my opinion, it's the same problem and I grew up around passing enums instead of raw true or false because of it. Could also use named locals or inline comments too. The former hasn't grown on me enough to do without thought and the latter tends to be the thing I reach for as a last resort.
> 
> FYI, you can always pass { } for all POD types + default constructible.
> 
> void f(bool, bool, const WebKit::EditingRange&, WebKit::EditingRange&&);
> 
> Compiler will happily let you call it f({ }, { }, { }, { }) <-- I don't know what is being passed. This is why I like to write out the types when there are consecutive default constructible things and why I don't like to initialize PODs using just { } <-- empty inside.
> 
> I would ❤️ to hear your thoughts.

Oh, just realized there won't be consecutive { } because of the { textAlternativeWithRange }. So the monotony is broken. So, there is less ambiguity here and while it is still a new thing for me, by the logic I stated above, I think (still feeling it) I can be at peace with passing { }.

[Daniel Bates]

Created attachment 394141 [details]
To Land

[Daniel Bates]

Committed r258796: <https://trac.webkit.org/changeset/258796>