[webkit-reviews] review granted: [Bug 190020] Search cancels but also clears highlights : [Attachment 351042] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Zamiul Haque <zhaque at apple.com>'s
request for review:
Bug 190020: Search cancels but also clears highlights
https://bugs.webkit.org/show_bug.cgi?id=190020

Attachment 351042: Patch

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




--- Comment #11 from Andy Estes <aestes at apple.com> ---
Comment on attachment 351042
  --> https://bugs.webkit.org/attachment.cgi?id=351042
Patch

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

This looks great Zamiul! r=me, but please address my comments before landing.

> Source/WebKit/ChangeLog:3
> +	   Search cancels but also clears highlights

This title doesn't really make sense. The issue isn't that we mistakenly
cleared highlights when the maximum count was exceeded, it's that we didn't
even cancel the search (because the only cancel API we had at the time also
cleared highlights).

> Source/WebKit/ChangeLog:10
> +	   When searching a PDF document on iOS, the maximum number of matching
> +	   terms are limited to 100. Beyond this limit, a PDF document should
not

Limiting the search to 100 items is what Safari happens to do, but other
clients might use a different limit. I'd talk about the search limit in more
generic terms and avoid mentioning the policy of a specific client.

> Source/WebKit/ChangeLog:17
> +	   In the process of making tests.

Do you plan to add tests directly to this patch?

If not, I'd mention that you are planning to test this in a follow-up. I think
a follow-up is fine since WKPDFView tests are currently disabled, and
re-enabling them on iOS 12 is a task in itself.

> Source/WebKit/UIProcess/ios/WKPDFView.mm:395
> +    if (numFound > _findStringMaxCount) {

It's probably my fault for writing a misleading FIXME comment, but I think you
want to stop when numFound is greater than or equal to _findStringMaxCount.

> Source/WebKit/UIProcess/ios/WKPDFView.mm:397
> +	   [controller cancelFindStringWithHighlightsCleared:false];
> +	   done = true;

For BOOL types we use YES and NO, not true and false.

> Source/WebKit/UIProcess/ios/WKPDFView.mm:402
> +    

Unneeded whitespace.