Bug 237408

Summary:

[iOS] Support PDF search when using a find interaction

Product:

WebKit

Reporter:

Aditya Keerthi <akeerthi>

Component:

New Bugs

Assignee:

Aditya Keerthi <akeerthi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, hi, katherine_cheney, megan_gardner, thorton, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	ews-feeder: commit-queue-
Patch	wenson_hsieh: review+
Patch for landing	none
Address post-landing feedback	none

Description Aditya Keerthi 2022-03-02 22:34:22 PST

...

Comment 1 Aditya Keerthi 2022-03-02 22:34:46 PST

rdar://89437334

Comment 2 Aditya Keerthi 2022-03-02 22:37:23 PST

Created attachment 453698 [details]
Patch

Comment 3 Aditya Keerthi 2022-03-02 22:56:08 PST

Created attachment 453699 [details]
Patch

Comment 4 Wenson Hsieh 2022-03-14 08:49:07 PDT

Comment on attachment 453699 [details]
Patch

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

> Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:787
> +        if ([_customContentView conformsToProtocol:@protocol(_UITextSearching)])
> +            [_findInteraction setSearchableObject:(id<_UITextSearching>)_customContentView.get()];
> +        else
> +            [_findInteraction setSearchableObject:_contentView.get()];

Nit - perhaps you could factor this out into a helper function, and use it both here and in `-_setFindInteractionEnabled:` below.

> Source/WebKit/UIProcess/ios/WKPDFView.mm:101
> +    auto pos = adoptNS([[WKPDFFoundTextPosition alloc] init]);

Nit - we usually don't abbreviate local variable names. Maybe rename to `position`?

Comment 5 Aditya Keerthi 2022-03-17 10:18:06 PDT

Created attachment 454986 [details]
Patch for landing

Comment 6 Aditya Keerthi 2022-03-17 10:18:43 PDT

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 453699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453699&action=review

Thanks for the review!
 
> > Source/WebKit/UIProcess/API/ios/WKWebViewIOS.mm:787
> > +        if ([_customContentView conformsToProtocol:@protocol(_UITextSearching)])
> > +            [_findInteraction setSearchableObject:(id<_UITextSearching>)_customContentView.get()];
> > +        else
> > +            [_findInteraction setSearchableObject:_contentView.get()];
> 
> Nit - perhaps you could factor this out into a helper function, and use it
> both here and in `-_setFindInteractionEnabled:` below.

Done.
 
> > Source/WebKit/UIProcess/ios/WKPDFView.mm:101
> > +    auto pos = adoptNS([[WKPDFFoundTextPosition alloc] init]);
> 
> Nit - we usually don't abbreviate local variable names. Maybe rename to
> `position`?

Done.

Comment 7 EWS 2022-03-17 14:59:07 PDT

Committed r291445 (248568@main): <https://commits.webkit.org/248568@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454986 [details].

Comment 8 Darin Adler 2022-03-21 13:14:52 PDT

Comment on attachment 454986 [details]
Patch for landing

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

> Source/WebKit/UIProcess/ios/WKPDFView.mm:718
> +    NSInteger offset = from.index - to.index;
> +
> +    if (offset < 0)
> +        return NSOrderedAscending;
> +
> +    if (offset > 0)
> +        return NSOrderedDescending;
> +
> +    return NSOrderedSame;

Probably not a real risk here, but typically doing subtraction to determine ordering can introduce overflow, so it’s better not to use that pattern.

Comment 9 Aditya Keerthi 2022-03-21 13:58:32 PDT

Reopening to attach new patch.

Comment 10 Aditya Keerthi 2022-03-21 13:58:33 PDT

Created attachment 455273 [details]
Address post-landing feedback

Comment 11 EWS 2022-03-21 17:23:17 PDT

Committed r291591 (248685@main): <https://commits.webkit.org/248685@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455273 [details].