Bug 150907 – Add preliminary (SPI) support for NSTextFinder on WKWebView

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Add preliminary (SPI) support for NSTextFinder on WKWebView

[Tim Horton]

Created attachment 264818 [details]
Patch

[WebKit Commit Bot]

Attachment 264818 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:242:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 264818 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:227
> +    _resultCollector(matchObjects.get(), NO);
> +    [_currentProgress setCompletedUnitCount:1];

Not safe to do this, could get multiple requests. Need to keep track of request/reply and which collector/progress we had.

[Tim Horton]

Created attachment 264885 [details]
Patch

[WebKit Commit Bot]

Attachment 264885 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:260:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:265:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 264897 [details]
Patch

[WebKit Commit Bot]

Attachment 264897 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:255:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:260:  Missing space before {  [whitespace/braces] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:107:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 264903 [details]
Patch

[WebKit Commit Bot]

Attachment 264903 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:255:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:260:  Missing space before {  [whitespace/braces] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:91:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:107:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 264903 [details]
Patch

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

> Source/WebKit2/Platform/spi/mac/AppKitSPI.h:26
> +#if PLATFORM(MAC)

Could do this where we import/include instead, just move it down to the PLATFORM(MAC) paragraph. Could use USE(APPKIT) instead. Not sure what our design pattern is for these Apple SPI.h headers.

> Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:49
> +- (void)didFindStringMatches:(const String&)string rects:(const Vector<Vector<IntRect>>&)rects index:(int32_t)matchIndex;
> +- (void)didFindString:(const String&)string rects:(const Vector<IntRect>&)rects index:(int32_t)matchIndex;
> +- (void)didFailToFindString:(const String&)string;

The string arguments are not used. Can we just omit that argument for now? Can always add later if needed.
The index arguments are not used.

Maybe we should put the conversion from WebCore types to Objective-C types in the C++ wrapper. The reason I suggest that is that selectors with C++ types in them actually cause a bit of binary bloat, so I generally prefer to use Objective-C types in Objective-C interfaces.

I typically like the “convert a vector into an NSArray” operation to be factored into its own separate function rather than being mixed in with other things.

> Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:52
> +- (void)didGetImageForMatchResult:(WebImage *)string index:(int32_t)matchIndex;

Again, index argument is not used.

> Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:81
> +    virtual void didFailToFindString(WebKit::WebPageProxy*, const WTF::String& string) override

No need for the WebKit:: and WTF:: prefixes here.

> Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:167
> +    _page = nullptr;
> +    _view = nil;

Should we do setFindMatchesClient(nullptr) and setFindClient(nullptr) here? If not, why is it safe not to?

> Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:230
> +    for (unsigned i = 0; i < matchCount; i++) {
> +        auto matchRects = rectsForMatches[i];

Modern for loop?

> Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:232
> +        for (auto rect : matchRects)

auto& instead of auto to avoid copying each IntRect?

> Source/WebKit2/WebProcess/WebPage/FindController.cpp:186
> +            range->absoluteTextRects(matchRects);

Does this function append to a vector rather than replacing the vector? If so, it has a misleading name! Seems likely that the person who wrote this function originally intended that it “fill” a vector and used append, which is why its name sounds like something with a return value yet it’s actually an function that appends to an existing vector. Maybe it should have append in its name.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:67
> +    FindInPageNavigationDelegate *delegate = [[FindInPageNavigationDelegate alloc] init];

This object leaks. Need to use ARC, adoptNS, release, or autorelease. Code one line above is using adoptNS.

[Tim Horton]

(In reply to comment #10)
> Comment on attachment 264903 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264903&action=review
> 
> > Source/WebKit2/Platform/spi/mac/AppKitSPI.h:26
> > +#if PLATFORM(MAC)
> 
> Could do this where we import/include instead, just move it down to the
> PLATFORM(MAC) paragraph. Could use USE(APPKIT) instead. Not sure what our
> design pattern is for these Apple SPI.h headers.

I like USE(APPKIT); not sure why it would be better to have the #if at all include sites, though. That seems backwards from what we usually do? But maybe not.

> > Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:49
> > +- (void)didFindStringMatches:(const String&)string rects:(const Vector<Vector<IntRect>>&)rects index:(int32_t)matchIndex;
> > +- (void)didFindString:(const String&)string rects:(const Vector<IntRect>&)rects index:(int32_t)matchIndex;
> > +- (void)didFailToFindString:(const String&)string;
> 
> The string arguments are not used. Can we just omit that argument for now?
> Can always add later if needed.
> The index arguments are not used.

Makes sense.

> > Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:81
> > +    virtual void didFailToFindString(WebKit::WebPageProxy*, const WTF::String& string) override
> 
> No need for the WebKit:: and WTF:: prefixes here.

True, here! We do need them for FindOptions, though, because we have things with the same name in WebCore and WebKit :D

> > Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm:232
> > +        for (auto rect : matchRects)
> 
> auto& instead of auto to avoid copying each IntRect?

Of course :(

> > Source/WebKit2/WebProcess/WebPage/FindController.cpp:186
> > +            range->absoluteTextRects(matchRects);
> 
> Does this function append to a vector rather than replacing the vector? If
> so, it has a misleading name! Seems likely that the person who wrote this
> function originally intended that it “fill” a vector and used append, which
> is why its name sounds like something with a return value yet it’s actually
> an function that appends to an existing vector. Maybe it should have append
> in its name.

It probably should have append in its name! It doesn't replace/clear, it does appendVector and friends.

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:67
> > +    FindInPageNavigationDelegate *delegate = [[FindInPageNavigationDelegate alloc] init];
> 
> This object leaks. Need to use ARC, adoptNS, release, or autorelease. Code
> one line above is using adoptNS.

Sure! This means there are other tests that do the wrong thing; I'll fix them too.

[Tim Horton]

> > > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:67
> > > +    FindInPageNavigationDelegate *delegate = [[FindInPageNavigationDelegate alloc] init];
> > 
> > This object leaks. Need to use ARC, adoptNS, release, or autorelease. Code
> > one line above is using adoptNS.
> 
> Sure! This means there are other tests that do the wrong thing; I'll fix
> them too.

Never mind! There are way too many, I'll just fix mine in this patch.

[Csaba Osztrogonác]

Patch landed in https://trac.webkit.org/changeset/192113.
Apple Mac CMake buildfix landed in https://trac.webkit.org/changeset/192152

Can we close this bug? Or is it open intentionally?