[webkit-reviews] review granted: [Bug 190621] [iOS] [Datalist] Can't pick datalist suggestions in a stock WKWebView : [Attachment 352762] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 190621: [iOS] [Datalist] Can't pick datalist suggestions in a stock
WKWebView
https://bugs.webkit.org/show_bug.cgi?id=190621

Attachment 352762: Patch

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




--- Comment #5 from Tim Horton <thorton at apple.com> ---
Comment on attachment 352762
  --> https://bugs.webkit.org/attachment.cgi?id=352762
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:253
> +    RetainPtr<UIView<WKFormControl>> _dataListTextSuggestionsInputView;
> +    RetainPtr<NSArray<UITextSuggestion *>> _dataListTextSuggestions;

Sooooooo I don't know if WebKit style has a preference, but I usually go with
"Class <ConformsToProtocol>" but "Class<GenericType>". AppKit public headers
seem to agree.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:398
> +    if (suggestions == _suggestions || [suggestions
isEqualToArray:_suggestions.get()])

This reads humorously but I think it's right since you want nil to compare
equal.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4805
> +    // Text suggestions vended from internal clients take precedence over
text suggestions from a focused form control with a datalist.

Why "internal" clients? What does that even mean? Aren't clients external by
definition?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4808
> +    if ([_formInputSession suggestions].count) {
> +	   [inputDelegate setSuggestions:[_formInputSession suggestions]];

If I'm reading this right, `suggestions` calls straight to the client, so you
should definitely only call it once and store the result here; no need to be so
chatty with the client!

> Tools/WebKitTestRunner/UIScriptControllerCocoa.mm:48
> +    [TestController::singleton().mainWebView()->platformView()
resignFirstResponder];

Do we ... fix this ... when the next test starts?