Bug 210398 – Move -_requestTextInputContextsInRect to WKContentView to simplify implementation

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Move -_requestTextInputContextsInRect to WKContentView to simplify implementation. -_requestTextInputContextsInRect is defined in WKWebView, but is not needed on Mac and this unnecessarily complicates its implementation: it has to deal with coordinate space differences due to differences in what is the root view on Mac and iOS + it has to know about iOS's custom content views. This can all be removed once moved to WKContentView.

[Radar WebKit Bug Importer]

<rdar://problem/61656931>

[Daniel Bates]

Created attachment 396205 [details]
Patch

[Daniel Bates]

Created attachment 396206 [details]
Patch

[Daniel Bates]

Created attachment 396207 [details]
Patch

[Daniel Bates]

Created attachment 396223 [details]
Patch

[Daniel Bates]

Created attachment 396224 [details]
Patch

[Daniel Bates]

Comment on attachment 396224 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:51
>  - (NSArray<_WKTextInputContext *> *)synchronouslyRequestTextInputContextsInRect:(CGRect)rect
>  {
>      __block bool finished = false;
>      __block RetainPtr<NSArray<_WKTextInputContext *>> result;
> -    [self _requestTextInputContextsInRect:rect completionHandler:^(NSArray<_WKTextInputContext *> *contexts) {
> +    [[self wkContentView] _requestTextInputContextsInRect:rect completionHandler:^(NSArray<_WKTextInputContext *> *contexts) {
>          result = contexts;
>          finished = true;
>      }];

This could be shared between this file and DocumentEditingContext.mm.

[Daniel Bates]

Created attachment 396226 [details]
Patch

[Darin Adler]

Comment on attachment 396226 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5064
> +- (void)_requestTextInputContextsInRect:(CGRect)rect completionHandler:(void(^)(NSArray<_WKTextInputContext *> *))completionHandler

Outside of the tests, where is the code that invokes this? Is this something defined by iOS that we are implementing/overriding?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm:53
>  @interface WKContentView ()
> +- (void)_requestTextInputContextsInRect:(CGRect)rect completionHandler:(void(^)(NSArray<_WKTextInputContext *> *))completionHandler;
>  - (void)requestDocumentContext:(UIWKDocumentRequest *)request completionHandler:(void (^)(UIWKDocumentContext *))completionHandler;
>  - (void)adjustSelectionWithDelta:(NSRange)deltaRange completionHandler:(void (^)(void))completionHandler;
>  @end

This doesn’t seem like a good way to expose methods for testing. Can we instead put this into a header accessible to the tests, like the PrivateForTesting ones?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:42
> +@interface WKContentView ()
> +- (void)_requestTextInputContextsInRect:(CGRect)rect completionHandler:(void(^)(NSArray<_WKTextInputContext *> *))completionHandler;
> +@end

Ditto.

[Daniel Bates]

Comment on attachment 396226 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5064
>> +- (void)_requestTextInputContextsInRect:(CGRect)rect completionHandler:(void(^)(NSArray<_WKTextInputContext *> *))completionHandler
> 
> Outside of the tests, where is the code that invokes this? Is this something defined by iOS that we are implementing/overriding?

Invoking code is in WebKitAdditions. It's something so that WebKitAdditions code can call this.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm:53
>>  @end
> 
> This doesn’t seem like a good way to expose methods for testing. Can we instead put this into a header accessible to the tests, like the PrivateForTesting ones?

OK

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/RequestTextInputContext.mm:42
>> +@end
> 
> Ditto.

OK

[Daniel Bates]

Created attachment 396240 [details]
Patch

[Daniel Bates]

Comment on attachment 396240 [details]
Patch

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

> Source/WebKit/UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h:33
> +@interface WKContentView (WKTestingIOS)

This is wrong. WKContentView is not defined and the WKContentView.h is a project header. We don't want to expose that because its an implementation detail. Could define stub WKContentView here, but it's already in TestWKWebView.h. so, I'll move this there.

[Daniel Bates]

Comment on attachment 396240 [details]
Patch

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

>> Source/WebKit/UIProcess/API/ios/WKWebViewPrivateForTestingIOS.h:33
>> +@interface WKContentView (WKTestingIOS)
> 
> This is wrong. WKContentView is not defined and the WKContentView.h is a project header. We don't want to expose that because its an implementation detail. Could define stub WKContentView here, but it's already in TestWKWebView.h. so, I'll move this there.

No, I'm not doing that either because I think it's going in the wrong direction: TestWebKitAPI should only test API or SPI so that it doesn't need to entangle itself in the guts of the dependent frameworks. I think that was the original intention of TestWebKitAPI. So, 'll expose new WKWebView testing SPI here and then WKWebViewPrivateForTestingIOS.cpp will have its implementation.

[Daniel Bates]

Created attachment 396241 [details]
Patch

[Daniel Bates]

Created attachment 396290 [details]
Patch

[Daniel Bates]

Comment on attachment 396290 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5065
> +    if (WebKit::mayContainEditableElementsInRect(self, rect)) {

This is wrong. This needs to be negated.

[Daniel Bates]

Created attachment 396344 [details]
For the bots

[Daniel Bates]

Created attachment 396349 [details]
For the bots

[Daniel Bates]

Created attachment 396441 [details]
Patch

[Daniel Bates]

Created attachment 396444 [details]
Patch

[Daniel Bates]

Comment on attachment 396444 [details]
Patch

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

> Source/WebKit/ChangeLog:26
> +        for the returned contexts to be in WKWebView coordinates. This keeps existings tests passing

existings => existing

> Source/WebKit/ChangeLog:27
> +        as mekes using this function intuitive since callers specify and get rects in WKWebView coordinates.

mekes => makes

[Daniel Bates]

Comment on attachment 396444 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        in what is the root view on Mac and iOS + it has to know about iOS's custom content views.

iOS's => iOS

[Darin Adler]

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

[Daniel Bates]

Comment on attachment 396444 [details]
Patch

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

Thanks for the review.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:-339
>> -- (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.

Your assumption is 100% correct: I did research these. These SPI are only used by WebKitAdditions: 0 other clients.

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

OK

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

I can add one... thought this was a one off, but I didn't grep, yet.

>> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:57
>> +            WebCore::ElementContext adjustedContext = context._textInputContext;
> 
> I’d suggest auto here:
> 
>     auto adjustedContext = context._textInputContext;

OK

>> 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());

yeah, I'll change.

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

Not necessary given I need to include the content view interaction header for -_requestTextInputContextsInRect. Will remove.

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

Will remove

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5102
>> +        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.

I knew you were working on this clean up and thought you were going to add it. When I wrote this patch I don't think you landed your change, yet. Is it landed now? If it's landed then I will try to add createNSArray() and use it.

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

OK

[Darin Adler]

Comment on attachment 396444 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:-339
>>> -- (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.
> 
> Your assumption is 100% correct: I did research these. These SPI are only used by WebKitAdditions: 0 other clients.

Should say this in change log. Something like: 'While these were in the "SPI" header, there is no software at Apple that was using these outside of WebKit, and I will assume no non-Apple software was using them either.'

>>> 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.
> 
> I can add one... thought this was a one off, but I didn't grep, yet.

Not saying you should do that; it’s OK to do it later or not at all. I’m just sort of shocked; seems like a huge pattern, "map", and surprised it’s not in Foundation.

>>> 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.
> 
> I knew you were working on this clean up and thought you were going to add it. When I wrote this patch I don't think you landed your change, yet. Is it landed now? If it's landed then I will try to add createNSArray() and use it.

I added createNSArray and VectorCocoa.h, but no, I have *not* added the createNSArray overload that takes a lambda. Haven’t even started on that yet.

[Daniel Bates]

Comment on attachment 396444 [details]
Patch

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

>>>> 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.
>> 
>> I can add one... thought this was a one off, but I didn't grep, yet.
> 
> Not saying you should do that; it’s OK to do it later or not at all. I’m just sort of shocked; seems like a huge pattern, "map", and surprised it’s not in Foundation.

OK. Quick grep for initWithCapacity:\w+\.count shows that a "transform" or map or whatever you want to call this helper function could be used in 4 places + this one (So 5 total). But where to put such a function?

>>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5102
>>>> +        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.
>> 
>> I knew you were working on this clean up and thought you were going to add it. When I wrote this patch I don't think you landed your change, yet. Is it landed now? If it's landed then I will try to add createNSArray() and use it.
> 
> I added createNSArray and VectorCocoa.h, but no, I have *not* added the createNSArray overload that takes a lambda. Haven’t even started on that yet.

Ok. I added it.

[Daniel Bates]

Comment on attachment 396444 [details]
Patch

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

>>>>> 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.
>>> 
>>> I can add one... thought this was a one off, but I didn't grep, yet.
>> 
>> Not saying you should do that; it’s OK to do it later or not at all. I’m just sort of shocked; seems like a huge pattern, "map", and surprised it’s not in Foundation.
> 
> OK. Quick grep for initWithCapacity:\w+\.count shows that a "transform" or map or whatever you want to call this helper function could be used in 4 places + this one (So 5 total). But where to put such a function?

Also should it be a standalone helper or on a category with a prefixed function name? If I do it, I'm leaning towards putting it in a category with a _web_ prefixed function name, thinking _web_transform (that's naming it after the C++ function). I would put it in WebCore/platform/cocoa/WebCoreNSArrayExtras.h

[Daniel Bates]

Comment on attachment 396444 [details]
Patch

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

>>>>>> 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.
>>>> 
>>>> I can add one... thought this was a one off, but I didn't grep, yet.
>>> 
>>> Not saying you should do that; it’s OK to do it later or not at all. I’m just sort of shocked; seems like a huge pattern, "map", and surprised it’s not in Foundation.
>> 
>> OK. Quick grep for initWithCapacity:\w+\.count shows that a "transform" or map or whatever you want to call this helper function could be used in 4 places + this one (So 5 total). But where to put such a function?
> 
> Also should it be a standalone helper or on a category with a prefixed function name? If I do it, I'm leaning towards putting it in a category with a _web_ prefixed function name, thinking _web_transform (that's naming it after the C++ function). I would put it in WebCore/platform/cocoa/WebCoreNSArrayExtras.h

I've decided I'm going to do ^^^ in a separate patch.

[Daniel Bates]

Created attachment 396656 [details]
To Land

[Daniel Bates]

Comment on attachment 396656 [details]
To Land

Clearing flags on attachment: 396656

Committed r260192: <https://trac.webkit.org/changeset/260192>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 396656 [details]
To Land

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

> Source/WTF/wtf/cocoa/VectorCocoa.h:64
> +template<typename VectorType, typename MapFunction> RetainPtr<NSArray> createNSArray(const VectorType& vector, MapFunction mapFunction)

Although this is already landed, some further feedback:

- I would have considered naming the template argument MapFunctionType since it’s not a function, it’s the type of a function. Note that the other templates here use that convention. Could also consider removing the word "Type" from those other template argument names, but I think it makes more sense to add the Type suffix, for here and now at least.

- I would have used the type const MapFunction& because there is no reason to copy the function.

- I would have named the argument function, because why not use one word instead of two in such a small, unambiguous context?

- Since the comment above carefully documents createNSArray and makeVector, should probably document this new overload too by reading the comment over, and adding a few words to it to cover this case.

Thanks for adding this, by the way!

> Source/WTF/wtf/cocoa/VectorCocoa.h:67
> +    auto size = vector.size();
> +    auto array = adoptNS([[NSMutableArray alloc] initWithCapacity:size]);

No need for a local variable here for size.

[Daniel Bates]

Comment on attachment 396656 [details]
To Land

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

>> Source/WTF/wtf/cocoa/VectorCocoa.h:64
>> +template<typename VectorType, typename MapFunction> RetainPtr<NSArray> createNSArray(const VectorType& vector, MapFunction mapFunction)
> 
> Although this is already landed, some further feedback:
> 
> - I would have considered naming the template argument MapFunctionType since it’s not a function, it’s the type of a function. Note that the other templates here use that convention. Could also consider removing the word "Type" from those other template argument names, but I think it makes more sense to add the Type suffix, for here and now at least.
> 
> - I would have used the type const MapFunction& because there is no reason to copy the function.
> 
> - I would have named the argument function, because why not use one word instead of two in such a small, unambiguous context?
> 
> - Since the comment above carefully documents createNSArray and makeVector, should probably document this new overload too by reading the comment over, and adding a few words to it to cover this case.
> 
> Thanks for adding this, by the way!

OK

>> Source/WTF/wtf/cocoa/VectorCocoa.h:67
>> +    auto array = adoptNS([[NSMutableArray alloc] initWithCapacity:size]);
> 
> No need for a local variable here for size.

There was no reason for the local in the above function either....

[Darin Adler]

Comment on attachment 396656 [details]
To Land

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

>>> Source/WTF/wtf/cocoa/VectorCocoa.h:67
>>> +    auto array = adoptNS([[NSMutableArray alloc] initWithCapacity:size]);
>> 
>> No need for a local variable here for size.
> 
> There was no reason for the local in the above function either....

Yes, should fix both!

[Daniel Bates]

Comment on attachment 396656 [details]
To Land

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

>>>> Source/WTF/wtf/cocoa/VectorCocoa.h:67
>>>> +    auto array = adoptNS([[NSMutableArray alloc] initWithCapacity:size]);
>>> 
>>> No need for a local variable here for size.
>> 
>> There was no reason for the local in the above function either....
> 
> Yes, should fix both!

OK I'll do that + I fix makeVector() as well: it over allocates and never corrects. Posted patch at bug #210610

[Daniel Bates]

Comment on attachment 396656 [details]
To Land

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

> Source/WebKit/UIProcess/API/ios/WKWebViewTestingIOS.mm:77
> +    return WebKit::mayContainEditableElementsInRect(_contentView.get(), [self convertRect:rect toView:_contentView.get()]);

This needs to be compile-time guarded behind ENABLE(EDITABLE_REGION). Fixed in <https://trac.webkit.org/changeset/260218>