Bug 150907 - Add preliminary (SPI) support for NSTextFinder on WKWebView
Summary: Add preliminary (SPI) support for NSTextFinder on WKWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-04 15:00 PST by Tim Horton
Modified: 2015-11-09 10:20 PST (History)
4 users (show)

See Also:


Attachments
Patch (31.66 KB, patch)
2015-11-04 15:01 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (32.73 KB, patch)
2015-11-05 14:38 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (45.27 KB, patch)
2015-11-05 15:21 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (46.27 KB, patch)
2015-11-05 17:10 PST, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-11-04 15:00:26 PST
Add preliminary (SPI) support for NSTextFinder on WKWebView
Comment 1 Tim Horton 2015-11-04 15:01:22 PST
Created attachment 264818 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-04 15:02:32 PST
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.
Comment 3 Tim Horton 2015-11-04 15:02:34 PST
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.
Comment 4 Tim Horton 2015-11-05 14:38:23 PST
Created attachment 264885 [details]
Patch
Comment 5 WebKit Commit Bot 2015-11-05 15:20:10 PST
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.
Comment 6 Tim Horton 2015-11-05 15:21:16 PST
Created attachment 264897 [details]
Patch
Comment 7 WebKit Commit Bot 2015-11-05 15:23:20 PST
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.
Comment 8 Tim Horton 2015-11-05 17:10:30 PST
Created attachment 264903 [details]
Patch
Comment 9 WebKit Commit Bot 2015-11-05 17:12:10 PST
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.
Comment 10 Darin Adler 2015-11-06 09:23:20 PST
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.
Comment 11 Tim Horton 2015-11-06 11:18:26 PST
(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.
Comment 12 Tim Horton 2015-11-06 11:44:09 PST
> > > 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.
Comment 13 Csaba Osztrogonác 2015-11-09 04:16:44 PST
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?