Bug 209308 - Have insertDictatedTextAsync() take an InsertTextOptions
Summary: Have insertDictatedTextAsync() take an InsertTextOptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-19 14:40 PDT by Daniel Bates
Modified: 2020-03-20 15:45 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.24 KB, patch)
2020-03-19 14:55 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (10.17 KB, patch)
2020-03-20 15:44 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-03-19 14:40:07 PDT
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.
Comment 1 Radar WebKit Bug Importer 2020-03-19 14:40:19 PDT
<rdar://problem/60652838>
Comment 2 Daniel Bates 2020-03-19 14:55:42 PDT
Created attachment 394025 [details]
Patch
Comment 3 Darin Adler 2020-03-20 10:58:58 PDT
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.
Comment 4 Daniel Bates 2020-03-20 15:36:04 PDT
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.
Comment 5 Daniel Bates 2020-03-20 15:41:27 PDT
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 { }.
Comment 6 Daniel Bates 2020-03-20 15:44:58 PDT
Created attachment 394141 [details]
To Land
Comment 7 Daniel Bates 2020-03-20 15:45:34 PDT
Committed r258796: <https://trac.webkit.org/changeset/258796>