[webkit-reviews] review granted: [Bug 210398] Move -_requestTextInputContextsInRect to WKContentView to simplify implementation : [Attachment 396444] 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 210398: Move -_requestTextInputContextsInRect to WKContentView to simplify
implementation
https://bugs.webkit.org/show_bug.cgi?id=210398

Attachment 396444: Patch

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




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

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:-339
> -- (BOOL)_mayContainEditableElementsInRect:(CGRect)rect;
> -- (void)_requestTextInputContextsInRect:(CGRect)rect
completionHandler:(void(^)(NSArray<_WKTextInputContext *> *))completionHandler
WK_API_AVAILABLE(macos(10.15), ios(13.0));

I’m surprised we can simply remove these.

Typically we can’t remove things from "Private.h" headers without making some
allowance for who might be calling on purpose or by accident. Two issues: 1)
Things, especially inside Apple, that will fail to compile because of the
header change. 2) Callers of these methods that will crash with a method not
found exception. In this case, I believe you are asserting that the only
callers we need to worry about are in internal Apple software and you have
researched and there are none that aren’t updated in lock step with WebKit?

Sometimes to mitigate issue (1) we leave the methods in the header with some
kind of deprecation annotation, and to mitigate issue (2) we leave the methods
in place and have them do nothing, to reduce risk. I am not doubting your
choice here, but I don’t see the explicit statement of how you determined this
was safe and neither of those were necessary. My intuition says you might be
right.

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:48
> +    CGRect adjustedRect = [self convertRect:rect toView:_contentView.get()];

I'd suggest auto here.

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:55
> +	   auto adjustedContexts = adoptNS([[NSMutableArray alloc]
initWithCapacity:contexts.count]);

I’m shocked that Foundation doesn’t offer a way to make one NSArray from
another using a block to describe how to map each element, but I can’t find
one. Seems like WTF should add one so that so we’re not writing loops like this
out. But still wondering if I just missed something.

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:57
> +	       WebCore::ElementContext adjustedContext =
context._textInputContext;

I’d suggest auto here:

    auto adjustedContext = context._textInputContext;

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:61
> +	   completionHandler(adjustedContexts.autorelease());

The use of autorelease here is unnecessary and arguably incorrect. Should be:

    completionHandler(adjustedContexts.get());

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:67
> +    [(id<UIWKInteractionViewProtocol>)_contentView
requestDocumentContext:request completionHandler:completionHandler];

Why is this cast needed? Isn’t there a header that exposes the fact that
WKWebView implements UIWKInteractionViewProtocol that the test can include?

If not, and we have to work around that, we could consider declaring a
WKWebView category to state this fact, rather than writing typecasts.

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:72
> +    [(id<UIWKInteractionViewProtocol>)_contentView
adjustSelectionWithDelta:deltaRange completionHandler:completionHandler];

Ditto.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5102
> +	   auto elements = adoptNS([[NSMutableArray alloc]
initWithCapacity:contexts.size()]);
> +	   for (const auto& context : contexts)
> +	       [elements addObject:adoptNS([[_WKTextInputContext alloc]
_initWithTextInputContext:context]).get()];
> +	   completionHandler(elements.get());

Should do as Sam Weinig suggested and add an overload of createNSArray in
VectorCocoa.h that takes a lambda, and then rewrite as follows:

    completionHandler(createNSArray(contexts, [] (const
WebCore::ElementContext& context) {
	return adoptNS([[_WKTextInputContext alloc]
_initWithTextInputContext:context]);
    }).get());

Maybe can even use auto& for the lambda argument type.

I’m not saying this needs to be done in this patch, but that should be our
future direction.

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:841
> +    CGRect contentRect = CGRectMake(x, y, width, height);

auto