Bug 239612

rdar://88442918

Created attachment 458078 [details]
Patch

Comment on attachment 458078 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:651
> +#if __has_include(<UIKit/UIFindInteraction.h>)

`HAVE` and other WTF macros are disallowed in framework headers.

Is it acceptable to leave a `__has_include` here, or do I need to write a post-processing script that removes this line on builds where it is safe to do so?

Comment on attachment 458078 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:651
>> +#if __has_include(<UIKit/UIFindInteraction.h>)
> 
> `HAVE` and other WTF macros are disallowed in framework headers.
> 
> Is it acceptable to leave a `__has_include` here, or do I need to write a post-processing script that removes this line on builds where it is safe to do so?

You should just be able to do @class UIFindInteraction;  Maybe inside a #if TARGET_OS_IPHONE

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1441
> +    return _findInteractionEnabled;

Would it be possible to have this value on the WebPageProxy instead of an ivar on the WKWebView?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1446
> +    if (_findInteractionEnabled != enabled) {

Ditto.  _page->setFindInteractionEnabled(enabled);
Then move this logic to WebPageProxyIOS.mm if possible.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:-496
> -@interface WKWebView (WKPrivateIOS) <_UITextSearching>

Do we still need _UITextSearching?

(In reply to Alex Christensen from comment #4)
> Comment on attachment 458078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458078&action=review
> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:651
> >> +#if __has_include(<UIKit/UIFindInteraction.h>)
> > 
> > `HAVE` and other WTF macros are disallowed in framework headers.
> > 
> > Is it acceptable to leave a `__has_include` here, or do I need to write a post-processing script that removes this line on builds where it is safe to do so?
> 
> You should just be able to do @class UIFindInteraction;  Maybe inside a #if
> TARGET_OS_IPHONE

Will do!
 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1441
> > +    return _findInteractionEnabled;
> 
> Would it be possible to have this value on the WebPageProxy instead of an
> ivar on the WKWebView?

We could move the value, but I'm not sure it makes sense to do so without also moving the setter – see below. 
 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1446
> > +    if (_findInteractionEnabled != enabled) {
> 
> Ditto.  _page->setFindInteractionEnabled(enabled);
> Then move this logic to WebPageProxyIOS.mm if possible.

The logic in this method creates a UIKit object, and calls methods on WKWebView. I'm not sure it makes sense to introduce `setFindInteractionEnabled` on `WebPageProxy`, given it will just result in us calling into PageClient and then back into WKWebView. Some indirection could be reduced if we moved the `_findInteraction` ivar into PageClient, but that class doesn't really know about UIKit, and it's still more indirection than the current approach.

What is the reasoning behind the desire to move the getter/setter into `WebPageProxy`?
 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:-496
> > -@interface WKWebView (WKPrivateIOS) <_UITextSearching>
> 
> Do we still need _UITextSearching?

Yes, internal clients still rely on our conformance to the `_UITextSearching` protocol, which contains a superset of the methods in `UITextSearching`.

Created attachment 458082 [details]
Patch

Comment on attachment 458082 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1466
> +#endif // HAVE(UIFINDINTERACTION)

You might want to have #else - (UIFindInteraction *)findInteraction { return nil; } - (BOOL)findInteractionEnabled { return NO; } - (void)setFindInteractionEnabled:(BOOL)enabled { }

Created attachment 458105 [details]
Patch

Created attachment 458106 [details]
Patch for landing

(In reply to Alex Christensen from comment #7)
> Comment on attachment 458082 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458082&action=review
>

Thanks for the review!
 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1466
> > +#endif // HAVE(UIFINDINTERACTION)
> 
> You might want to have #else - (UIFindInteraction *)findInteraction { return
> nil; } - (BOOL)findInteractionEnabled { return NO; } -
> (void)setFindInteractionEnabled:(BOOL)enabled { }

Makes sense. I've put the `#else` inside the existing method definitions rather than reimplementing them, matching other methods that use the same approach.

Created attachment 458108 [details]
Patch for landing

Created attachment 458110 [details]
Patch for landing

Committed r293231 (249895@main): <https://commits.webkit.org/249895@main>

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