Bug 235830 – Add SPI to retrieve the rect of a found text range

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Aditya Keerthi]

...

[Aditya Keerthi]

rdar://88193004

[Aditya Keerthi]

Created attachment 450255 [details]
Patch

[Aditya Keerthi]

Created attachment 450264 [details]
Patch

[Darin Adler]

Comment on attachment 450264 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:10145
> +    WKFoundTextRange *foundTextRange = (WKFoundTextRange *)range;

Local variable doesn’t add much value and involves repeating WKFoundTextRange twice, which we could mitigate with auto. Also, could use dynamic_objc_cast or checked_objc_cast if you like; they are for this.

> Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:184
> +    auto rect = frameView->contentsToRootView(WebCore::unionRect(WebCore::RenderObject::absoluteTextRects(*simpleRange)));

local variable doesn’t help here

We may not need the WebCore prefix on the call unionRect because of argument-dependent lookup, since the type is in the WebCore namespace.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:410
> +    auto searchOptions = adoptNS([[TestTextSearchOptions alloc] init]);

Would just name this local "options".

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:411
> +    [webView performTextSearchWithQueryString:query usingOptions:(_UITextSearchOptions *)searchOptions.get() resultAggregator:aggregator.get()];

Why is a typecast to _UITextSearchOptions needed here? That does not seem right!

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:500
> +    RetainPtr<NSArray<UITextRange *>> ranges = adoptNS(getTextRangesForQueryString(webView.get(), @"Sapien"));

I suggest auto here.

[Aditya Keerthi]

Created attachment 450273 [details]
Patch

[Aditya Keerthi]

(In reply to Darin Adler from comment #4)
> Comment on attachment 450264 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450264&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:10145
> > +    WKFoundTextRange *foundTextRange = (WKFoundTextRange *)range;
> 
> Local variable doesn’t add much value and involves repeating
> WKFoundTextRange twice, which we could mitigate with auto. Also, could use
> dynamic_objc_cast or checked_objc_cast if you like; they are for this.

Did not know about `dynamic_objc_cast`! Decided to go with that.
 
> > Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:184
> > +    auto rect = frameView->contentsToRootView(WebCore::unionRect(WebCore::RenderObject::absoluteTextRects(*simpleRange)));
> 
> local variable doesn’t help here
> 
> We may not need the WebCore prefix on the call unionRect because of
> argument-dependent lookup, since the type is in the WebCore namespace.

Removed local variable and prefix.
 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:410
> > +    auto searchOptions = adoptNS([[TestTextSearchOptions alloc] init]);
> 
> Would just name this local "options".

Done.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:411
> > +    [webView performTextSearchWithQueryString:query usingOptions:(_UITextSearchOptions *)searchOptions.get() resultAggregator:aggregator.get()];
> 
> Why is a typecast to _UITextSearchOptions needed here? That does not seem
> right!

The typecast is because UIKit doesn't export this symbol yet. I've added a comment and linked to the corresponding bug.
 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:500
> > +    RetainPtr<NSArray<UITextRange *>> ranges = adoptNS(getTextRangesForQueryString(webView.get(), @"Sapien"));
> 
> I suggest auto here.

Done.

[Wenson Hsieh]

Comment on attachment 450273 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:343
> +@property (nonatomic, readonly) NSArray<UITextRange *>* foundRanges;

Nit - the *s are inconsistent here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:501
> +    auto ranges = adoptNS(getTextRangesForQueryString(webView.get(), @"Sapien"));

Nit - functions that return +1 Objective-C ids usually start with names like "copy" or "new".

Perhaps, `copyTextRangesForQueryString`? (Or you could just move the adoptNS into the helper function itself, and return a RetainPtr instead).

[Aditya Keerthi]

Created attachment 450286 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Wenson Hsieh from comment #7)
> Comment on attachment 450273 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450273&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:343
> > +@property (nonatomic, readonly) NSArray<UITextRange *>* foundRanges;
> 
> Nit - the *s are inconsistent here.

Fixed.
 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/FindInPage.mm:501
> > +    auto ranges = adoptNS(getTextRangesForQueryString(webView.get(), @"Sapien"));
> 
> Nit - functions that return +1 Objective-C ids usually start with names like
> "copy" or "new".
> 
> Perhaps, `copyTextRangesForQueryString`? (Or you could just move the adoptNS
> into the helper function itself, and return a RetainPtr instead).

Moved the adoptNS into the helper function.

[EWS]

Committed r288773 (246554@main): <https://commits.webkit.org/246554@main>

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