Bug 143065 – Implement iOS WebKit2 PDF Find-in-Page

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

[Tim Horton]

Implement iOS WebKit2 PDF Find-in-Page

[Tim Horton]

Created attachment 249444 [details]
Patch

[WebKit Commit Bot]

Attachment 249444 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:478:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:526:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKFindOptions.h:30:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 249546 [details]
Patch

[WebKit Commit Bot]

Attachment 249546 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:478:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:526:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKFindOptions.h:30:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 249546 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:402
> +- (NSStringCompareOptions)_stringCompareOptionsFromWKFindOptions:(_WKFindOptions)options
> +{
> +    NSStringCompareOptions findOptions = 0;
> +    if (options & _WKFindOptionsCaseInsensitive)
> +        findOptions |= NSCaseInsensitiveSearch;
> +    if (options & _WKFindOptionsBackwards)
> +        findOptions |= NSBackwardsSearch;
> +    return findOptions;
> +}

Seem like this can be a static function.

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:404
> +- (void)_computeMatchesForString:(NSString *)string options:(_WKFindOptions)options maxCount:(NSUInteger)maxCount completionHandler:(void (^)(bool success))completionHandler

Not BOOL?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:411
> +    _WKFindOptions optionsAffectingResults = options & ~(_WKFindOptionsBackwards | _WKFindOptionsShowOverlay | _WKFindOptionsShowFindIndicator | _WKFindOptionsShowHighlight | _WKFindOptionsWrapAround | _WKFindOptionsDetermineMatchIndex);

Maybe this should be defined in _WKFindOptions.h so that when new options are added the person adding them can remember to update this value if needed.

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:432
> +    unsigned computeMatchesOperationID = _nextComputeMatchesOperationID++;

I’d ++_nextComputedMatchesOperationID here…(*)

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:437
> +    dispatch_async(_findQueue, ^{

I think we prefer lambdas with explicit captures to blocks in C++ code.

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:440
> +        for (unsigned pageIndex = 0; pageIndex < pages.size(); pageIndex++) {

post++!

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:448
> +                if (_nextComputeMatchesOperationID > computeMatchesOperationID + 1) {

* …and do a simple != here.

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:460
> +            if (_nextComputeMatchesOperationID > computeMatchesOperationID + 1)

and here.

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:496
> +            CGRect zoomRect = [pageInfo.view convertRectFromPDFPageSpace:[match bounds]];

.bounds

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:517
> +            NSUInteger location = [_currentFindSelection startIndex];

Can this be zero?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:518
> +            previousFindSelection = adoptNS([[UIPDFSelection alloc] initWithPage:[_currentFindSelection page] fromIndex:location - 1 toIndex:location]);

What if location is zero?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:531
> +        for (unsigned i = 0; i < _pages.size(); i++) {

post++ again?

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:532
> +            UIPDFSelection *match = [_pages[pageIndex].page findString:string fromSelection:(pageIndex == previousFindPageIndex ? previousFindSelection.get() : nullptr) options:findOptions];

nil

[Tim Horton]

(In reply to comment #5)
> Comment on attachment 249546 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249546&action=review
> > Source/WebKit2/UIProcess/ios/WKPDFView.mm:411
> > +    _WKFindOptions optionsAffectingResults = options & ~(_WKFindOptionsBackwards | _WKFindOptionsShowOverlay | _WKFindOptionsShowFindIndicator | _WKFindOptionsShowHighlight | _WKFindOptionsWrapAround | _WKFindOptionsDetermineMatchIndex);
> 
> Maybe this should be defined in _WKFindOptions.h so that when new options
> are added the person adding them can remember to update this value if needed.

I'm not sure, because whether "Backwards" affects results or not depends on what you're doing. I do agree that it would be unfortunate if new options were added and not added to this list, though. Maybe _WKFindOptions.h can have the list except Backwards and we can add Backwards in?

[Tim Horton]

Created attachment 249609 [details]
Patch

[WebKit Commit Bot]

Attachment 249609 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:487:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/WKPDFView.mm:536:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKFindOptions.h:30:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 249611 [details]
Patch

[Tim Horton]

http://trac.webkit.org/changeset/182085