Bug 165801 – [Mac] -[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:] invokes the resultCollector with didWrap = NO even when it wraps

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

-[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:] invokes the resultCollector with didWrap = NO even when it wraps.

<rdar://problem/29649535>

[Tim Horton]

Created attachment 306030 [details]
Patch

[Build Bot]

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


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


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

Comment on attachment 306030 [details]
Patch

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

> Source/WebCore/page/FrameTree.h:69
> -    WEBCORE_EXPORT Frame* traverseNextWithWrap(bool) const;
> -    WEBCORE_EXPORT Frame* traversePreviousWithWrap(bool) const;
> +    WEBCORE_EXPORT Frame* traverseNextWithWrap(CanWrap, DidWrap* = nullptr) const;
> +    WEBCORE_EXPORT Frame* traversePreviousWithWrap(CanWrap, DidWrap* = nullptr) const;

The “withWrap” part of the function name was intended to described the boolean argument. Now that the argument is self-explanatory, you could drop “withWrap” from these and other function names.

[Tim Horton]

(In reply to [e-mail] from comment #4)
> Comment on attachment 306030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306030&action=review
> 
> > Source/WebCore/page/FrameTree.h:69
> > -    WEBCORE_EXPORT Frame* traverseNextWithWrap(bool) const;
> > -    WEBCORE_EXPORT Frame* traversePreviousWithWrap(bool) const;
> > +    WEBCORE_EXPORT Frame* traverseNextWithWrap(CanWrap, DidWrap* = nullptr) const;
> > +    WEBCORE_EXPORT Frame* traversePreviousWithWrap(CanWrap, DidWrap* = nullptr) const;
> 
> The “withWrap” part of the function name was intended to described the
> boolean argument. Now that the argument is self-explanatory, you could drop
> “withWrap” from these and other function names.

That's a good point.

[Tim Horton]

Created attachment 306089 [details]
Patch

[Build Bot]

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


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


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

[Wenson Hsieh]

Comment on attachment 306089 [details]
Patch

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

> Source/WebCore/page/FrameTree.cpp:420
> +Frame* FrameTree::traversePrevious(CanWrap canWrap, DidWrap* didWrap) const

Should this set *didWrap = DidWrap::No at the top to ensure that didWrap gets set to something if it is nonnull?

[Wenson Hsieh]

Comment on attachment 306089 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:224
> +    [webView findMatchesForString:@"word" relativeToMatch:nil findOptions:defaultFindOptions maxResults:1 resultCollector:^(NSArray *matches, BOOL didWrap) {

Minor nit - I would factor out the call to findMatchesForString into a helper to make these calls more readable (and maybe wait for each one to complete before moving on so we don't need to nest each step in the test in a new block).

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:249
> +    findMatchesDone = false;

Side note: I think it's a bit weird that findMatchesDone is a static variable here :/ I think we should just make it local to each test, but that could be for another patch later. Also, do we need to set it to false here? I thought since each test is run in a different process, this state should not bleed into any subsequent tests.

[Tim Horton]

https://trac.webkit.org/changeset/214893/webkit