Bug 215816 – [iOS] provide a way to get previously inserted alternatives for the selected text

• Component: New Bugs
• Status: RESOLVED FIXED

[Devin Rousso]

`-[WKContentView insertText:alternatives:style:]` propagates the `NSArray<NSString *> *alternatives` to the WebProcess for showing markers in the inserted text, but has no way of providing the caller back the `NSArray<NSString *> *alternatives` it gave to the `WKContentView`

[Devin Rousso]

<rdar://problem/66646042>

[Devin Rousso]

Created attachment 407210 [details]
Patch

[Devin Rousso]

Created attachment 407242 [details]
Patch

[Darin Adler]

Comment on attachment 407242 [details]
Patch

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

Looks like the test is failing on the bot, so I won’t say review+ yet since I assume you’ll have to make changes to get the test passing.

> Source/WebKit/UIProcess/PageClient.h:381
> +    virtual NSTextAlternatives *platformDictationAlternatives(WebCore::DictationContext) = 0;

Not thrilled by the use of the word "platform" here. How about putting the word "cocoa" in the name instead?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3125
> +    if (dictationContextsForSelection.isEmpty())
> +        return @[ ];

Is it important to optimize this case? The code below will also create an empty array so you could just leave this out unless it’s a valuable performance optimization.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:321
> +        if (!markers.isEmpty()) {

Why the check for empty vector? The code below will correctly handle an empty vector. I suggest leaving this if statement out.

> Tools/TestWebKitAPI/ios/UIKitSPI.h:250
> +@protocol UIWKInteractionViewProtocol_66646042 <UIWKInteractionViewProtocol>
> +- (NSArray<NSTextAlternatives *> *)alternativesForSelectedText;
> +@end

Do we really need to specify this peculiar protocol name, UIWKInteractionViewProtocol_66646042, to have this work correctly? Why? I don’t think it’s a good idea to have a name with a Radar bug number in it.

[Devin Rousso]

Comment on attachment 407242 [details]
Patch

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

>> Source/WebKit/UIProcess/PageClient.h:381
>> +    virtual NSTextAlternatives *platformDictationAlternatives(WebCore::DictationContext) = 0;
> 
> Not thrilled by the use of the word "platform" here. How about putting the word "cocoa" in the name instead?

My thinking was that all of the other functions didn't use anything platform-specific, so if other platforms decided to leverage this then they could override `plaftormDictationAlternatives` and be able to use the same name.  I've not seen functions with "cocoa" in the name before, so that seems a bit odd to me personally.  I have no preference as to how this is named tho, so I will take any suggestions.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3125
>> +        return @[ ];
> 
> Is it important to optimize this case? The code below will also create an empty array so you could just leave this out unless it’s a valuable performance optimization.

good point

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:321
>> +        if (!markers.isEmpty()) {
> 
> Why the check for empty vector? The code below will correctly handle an empty vector. I suggest leaving this if statement out.

good point

>> Tools/TestWebKitAPI/ios/UIKitSPI.h:250
>> +@end
> 
> Do we really need to specify this peculiar protocol name, UIWKInteractionViewProtocol_66646042, to have this work correctly? Why? I don’t think it’s a good idea to have a name with a Radar bug number in it.

I was told this is how to do this for things that are not in any SDK so that we know when to move this code into the non-`USE(APPLE_INTERNAL_SDK)` area.  I'll tweak the name slightly to make this clearer.

[Tim Horton]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 407242 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407242&action=review
> 
> >> Tools/TestWebKitAPI/ios/UIKitSPI.h:250
> >> +@end
> > 
> > Do we really need to specify this peculiar protocol name, UIWKInteractionViewProtocol_66646042, to have this work correctly? Why? I don’t think it’s a good idea to have a name with a Radar bug number in it.
> 
> I was told this is how to do this for things that are not in any SDK so that
> we know when to move this code into the non-`USE(APPLE_INTERNAL_SDK)` area. 
> I'll tweak the name slightly to make this clearer.

This is fairly typical internally, not so much (in my experience) in WebKit.

[Devin Rousso]

Created attachment 407437 [details]
Patch

[Devin Rousso]

Created attachment 407440 [details]
Patch

[Darin Adler]

Comment on attachment 407440 [details]
Patch

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

> Source/WebKit/UIProcess/PageClient.h:381
> +    virtual NSTextAlternatives *platformDictationAlternatives(WebCore::DictationContext) = 0;

I understand that I did not convince you to change this.

But "platform" is a very poor prefix to use to return a Cocoa-specific version of a data structure. We have been using "platform" in multiple inconsistent ways and it’s making our code hard to understand. The word "platform" itself is ambiguous. And there’s no reason to be vague about what platform we mean unless it’s a cross-platform abstraction. This never returns the dictation alternatives of any other platform, so it’s platform-specific for Cocoa.

By naming it "platform" we leave the incorrect impression that other platforms should some day implement this, and general spread a vague sense of uncertainty about what this function does.

[EWS]

Committed r266265: <https://trac.webkit.org/changeset/266265>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407440 [details].

[Devin Rousso]

Comment on attachment 407440 [details]
Patch

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

>> Source/WebKit/UIProcess/PageClient.h:381
>> +    virtual NSTextAlternatives *platformDictationAlternatives(WebCore::DictationContext) = 0;
> 
> I understand that I did not convince you to change this.
> 
> But "platform" is a very poor prefix to use to return a Cocoa-specific version of a data structure. We have been using "platform" in multiple inconsistent ways and it’s making our code hard to understand. The word "platform" itself is ambiguous. And there’s no reason to be vague about what platform we mean unless it’s a cross-platform abstraction. This never returns the dictation alternatives of any other platform, so it’s platform-specific for Cocoa.
> 
> By naming it "platform" we leave the incorrect impression that other platforms should some day implement this, and general spread a vague sense of uncertainty about what this function does.

I apologize if that was the way this was interpreted.  That was not my intention.  I put up the most recent patch as both a check to see if I fixed the issue with the bots (yay!) and as another opportunity for people to review the code.  I was still open to suggestions and would've happily discussed this further if you felt strongly.  I interpreted your original comment as a suggestion, which I responded to with my own thoughts on the subject, but I was not set in stone or anything like that.  I'd hoped that my "I have no preference as to how this is named tho, so I will take any suggestions." comment on the last patch would conveyed this, but perhaps my commit-queue? made you think that I was not willing to change.  If so, I apologize.

To respond to your most recent comment, I see many uses of `platform*()` across WebKit and see that as a way of getting the underlying platform representation of some "thing" or doing some work that requires specific platform functionality/idioms.  In the case of this code, although `USE(DICTATION_ALTERNATIVES)` is indeed only enable for `PLATFORM(COCOA)`, there is nothing about `USE(DICTATION_ALTERNATIVES)` that is indicative of being cocoa-only when it's used.  Yes, no other platforms are using `USE(DICTATION_ALTERNATIVES)` right now, but that doesn't mean they won't/can't in the future.  Perhaps I should've wrapped `platformDictationAlternatives` in a `#if PLATFORM(COCOA)` to make that clearer.  I fully admit that I could be wrong/mistaken/confused/etc. in my understanding/view of this.  If there is indeed an implication/impression from engineers on other platforms that they should implement this then I will change it.  I'm not trying to do that.  But at the same time, I don't want to write code that makes them think the opposite.

If you feel strongly that this should be named something else, I will happily change it.  I am not trying to be/look stubborn 🙂

Regardless, thanks for the review and feedback 😁