Bug 237408 – [iOS] Support PDF search when using a find interaction

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

[Aditya Keerthi]

...

[Aditya Keerthi]

rdar://89437334

[Aditya Keerthi]

Created attachment 453698 [details]
Patch

[Aditya Keerthi]

Created attachment 453699 [details]
Patch

[Wenson Hsieh]

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`?

[Aditya Keerthi]

Created attachment 454986 [details]
Patch for landing

[Aditya Keerthi]

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

[EWS]

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

[Darin Adler]

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.

[Aditya Keerthi]

Reopening to attach new patch.

[Aditya Keerthi]

Created attachment 455273 [details]
Address post-landing feedback

[EWS]

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