[webkit-reviews] review granted: [Bug 165801] [Mac] -[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:] invokes the resultCollector with didWrap = NO even when it wraps : [Attachment 306089] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 165801: [Mac] -[WKWebView
findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:]
invokes the resultCollector with didWrap = NO even when it wraps
https://bugs.webkit.org/show_bug.cgi?id=165801

Attachment 306089: Patch

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




--- Comment #9 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 306089
  --> https://bugs.webkit.org/attachment.cgi?id=306089
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.