Bug 190020 – Search cancels but also clears highlights

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zamiul Haque]

Search cancels but also clears highlights

[Zamiul Haque]

Created attachment 350929 [details]
Patch

[Zamiul Haque]

<rdar://problem/39546973>

[Andy Estes]

Comment on attachment 350929 [details]
Patch

Thanks for the patch, Zamiul!

A few questions/comments:

1. How did you test this?
2. You'll notice that the ios and ios-sim EWS bots are red. I think this is because you didn't declare -cancelFindStringWithHighlightsCleared: in PDFKitSPI.h.
3. I think you'll need to still update _findStringCount and call _findCompletion when _findStringMaxCount is exceeded. Otherwise, we'll never inform the client about the find count, never focus the first result, and probably won't handle wrap-around properly.

[Andy Estes]

Also note that we have API tests for PDF find-in page in Tools/TestWebKitAPI/Tests/WebKitCocoa/WKPDFView.mm. Right now those tests are disabled on iOS 12, and rdar://problem/39475542 tracks fixing that. Maybe you can help with that too!

[Andy Estes]

Comment on attachment 350929 [details]
Patch

r- for now based on my previous comments.

[Zamiul Haque]

(In reply to Andy Estes from comment #3)
> Comment on attachment 350929 [details]
> Patch
> 
> Thanks for the patch, Zamiul!
> 
> A few questions/comments:
> 
> 1. How did you test this?
> 2. You'll notice that the ios and ios-sim EWS bots are red. I think this is
> because you didn't declare -cancelFindStringWithHighlightsCleared: in
> PDFKitSPI.h.
> 3. I think you'll need to still update _findStringCount and call
> _findCompletion when _findStringMaxCount is exceeded. Otherwise, we'll never
> inform the client about the find count, never focus the first result, and
> probably won't handle wrap-around properly.

1. I tested it manually by changing _maximumNumberOfMatches in _SFFindOnPageView.m to 4, compiling it to the iOS simulator (iPhone X) and searching a PDF document for a string that occurs zero times, once, 3 times, 4 times and more than 4 times. I also tested the macOS PDF find-in-page feature, just in case something was broken. I was also relying on ews to give me any hints, if something went wrong.

2 & 3. I've changed the code to attend to both of these concerns. Please let me know what you think.

I'll attend to making proper test cases for this now.

[Zamiul Haque]

(In reply to Andy Estes from comment #4)
> Also note that we have API tests for PDF find-in page in
> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKPDFView.mm. Right now those tests
> are disabled on iOS 12, and rdar://problem/39475542 tracks fixing that.
> Maybe you can help with that too!

Yeah, this looks helpful. I'll look into this and base my test cases for this bug off of it.

[Wenson Hsieh]

Comment on attachment 350929 [details]
Patch

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

> Source/WebKit/ChangeLog:5
> +        <rdar://problem/39546973>

Nit - I think you should leave out this radar, since it only represents PDFKit work blocking this bug (<rdar://problem/39585214>)

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

Another very minor nit — extra newline here.

[Zamiul Haque]

Created attachment 351042 [details]
Patch

[Zamiul Haque]

(In reply to Wenson Hsieh from comment #8)
> Comment on attachment 350929 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350929&action=review
> 
> > Source/WebKit/ChangeLog:5
> > +        <rdar://problem/39546973>
> 
> Nit - I think you should leave out this radar, since it only represents
> PDFKit work blocking this bug (<rdar://problem/39585214>)
> 
> > Source/WebKit/UIProcess/ios/WKPDFView.mm:398
> > +        
> 
> Another very minor nit — extra newline here.

Done and done!

[Andy Estes]

Comment on attachment 351042 [details]
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.

[Andy Estes]

Comment on attachment 351042 [details]
Patch

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

>> 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.

One more comment about this: if numFound happens to be greater than or equal to _findStringMaxCount but done is also YES, then we probably don't want to call -cancelFindStringWithHighlightsCleared:.

[Zamiul Haque]

Created attachment 351557 [details]
Patch

[Zamiul Haque]

(In reply to Andy Estes from comment #12)
> Comment on attachment 351042 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351042&action=review
> 
> >> 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.
> 
> One more comment about this: if numFound happens to be greater than or equal
> to _findStringMaxCount but done is also YES, then we probably don't want to
> call -cancelFindStringWithHighlightsCleared:.

True, I've included that in the conditional.

[Zamiul Haque]

(In reply to Andy Estes from comment #11)
> Comment on attachment 351042 [details]
> 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.

As per our discussion, we decided to stick with numFound > _findStringMaxCount, rather than use a >=. The latter would finish a bit too early and result in unexpected UI updates.

[WebKit Commit Bot]

Comment on attachment 351557 [details]
Patch

Clearing flags on attachment: 351557

Committed r236815: <https://trac.webkit.org/changeset/236815>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.