...
rdar://88193004
Created attachment 450255 [details] Patch
Created attachment 450264 [details] Patch
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.
Created attachment 450273 [details] Patch
(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.
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).
Created attachment 450286 [details] Patch for landing
(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.
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].