Bug 57765 – WK2: PDF: Find in page

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Jer Noble]

We don't support find in page when displaying PDFs

[Jer Noble]

<rdar://problem/8750336>

[Jer Noble]

Created attachment 88084 [details]
Patch

[Jer Noble]

Created attachment 88146 [details]
Patch

Re-upload after rebasing; this should fix the buildire's inability to merge.

[WebKit Review Bot]

Attachment 88084 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8332125

[Jer Noble]

Whoops, no need for those new functions on PDFViewController to be virtual.

[Early Warning System Bot]

Attachment 88146 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8333096

[Jer Noble]

Created attachment 88147 [details]
Patch

[Early Warning System Bot]

Attachment 88147 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8336053

[Jer Noble]

Created attachment 88151 [details]
Patch

[Build Bot]

Attachment 88151 [details] did not build on win:
Build output: http://queues.webkit.org/results/8334127

[Maciej Stachowiak]

Comment on attachment 88151 [details]
Patch

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

Looks like this is still breaking the windows build according to EWS, so r- for that. Other than that, I don't see any obvious showstopper problems.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:235
> +        // Initially we want to include the selected text in the search. PDFDocument's API always searches from just
> +        // past the passed-in selection, so we need to pass a selection that's modified appropriately.
> +        // FIXME 4182863: Ideally we'd use a zero-length selection at the edge of the current selection, but zero-length
> +        // selections don't work in PDFDocument. So instead we make a one-length selection just before or after the
> +        // current selection, which works for our purposes even when the current selection is at an edge of the
> +        // document.

This comment is a bit verbose, any way to make it shorter?

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 88151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88151&action=review
> 
> Looks like this is still breaking the windows build according to EWS, so r- for that. Other than that, I don't see any obvious showstopper problems.

I'll get a new patch put up with a fix for the mac build.

> > Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:235
> > +        // Initially we want to include the selected text in the search. PDFDocument's API always searches from just
> > +        // past the passed-in selection, so we need to pass a selection that's modified appropriately.
> > +        // FIXME 4182863: Ideally we'd use a zero-length selection at the edge of the current selection, but zero-length
> > +        // selections don't work in PDFDocument. So instead we make a one-length selection just before or after the
> > +        // current selection, which works for our purposes even when the current selection is at an edge of the
> > +        // document.
> 
> This comment is a bit verbose, any way to make it shorter?

Probably. :)

It was copied in with the rest of the implementation from WebPDFView in WebKit/.  It was helpful to me when figuring out what this function was doing, but I could un-verbosify it.

[Jer Noble]

Created attachment 88747 [details]
Patch

Added stubs to the windows PageClient.h subclass.

[Early Warning System Bot]

Attachment 88747 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8342721

[Build Bot]

Attachment 88747 [details] did not build on win:
Build output: http://queues.webkit.org/results/8373365

[Eric Seidel (no email)]

Attachment 88747 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8374035

[Jer Noble]

Drat, part of my patch got merged out of existence.  I'll re-upload.

[Jer Noble]

Created attachment 88829 [details]
Patch

[Early Warning System Bot]

Attachment 88829 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8377062

[Jer Noble]

Somehow managed to merge my Qt changes out of existence as well. :-/

[Build Bot]

Attachment 88829 [details] did not build on win:
Build output: http://queues.webkit.org/results/8377063

[Jer Noble]

::Sigh::  This patch is not going well.  Waiting for the mac and gtk bots to complete to make sure I don't have any /other/ errors in my patch before re-uploading.

[WebKit Review Bot]

Attachment 88829 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8374175

[Jer Noble]

Created attachment 88849 [details]
Patch

Comment on attachment 88849 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:91
> +    int count = [aPages count];
> +    int i;
> +    for (i = 0; i < count; ++i) {

In Objective-C++, you’re allow to define i inside the for statement.  count and i can also be an NSUIntegers.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:96
> +        if (!NSEqualRects(aBounds, bBounds)) {
> +            return NO;
> +        }

No braces around single-line blocks please.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:118
> +- (PDFSelection *)_nextMatchFor:(NSString *)string direction:(BOOL)forward caseSensitive:(BOOL)caseFlag wrap:(BOOL)wrapFlag fromSelection:(PDFSelection *)initialSelection startInSelection:(BOOL)startInSelection;

Search methods with multiple flags are old-school. The new preferred way is to use a single options parameter. But I guess this is fine.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:243
> +    [selectionForInitialSearch release];

You could use a RetainPtr instead of manually releasing here.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:255
> +- (NSUInteger)_countMatches:(NSString*)string caseSensitive:(BOOL)caseFlag

Missing space after “NSString”

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:258
> +        return nil;

Shouldn’t return nil from a method with an NSUInteger return type.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:624
> +    page()->didFindString(string, matchCount);

This function ignores maxMatchCount (I wonder why the compiler isn’t complaining). Shouldn’t it do the same thing FindController does, namely:

            // Check if we have more matches than allowed.
            if (matchCount > maxMatchCount)
                matchCount = static_cast<unsigned>(kWKMoreThanMaximumMatchCount);

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:632
> +    page()->didCountStringMatches(string, matchCount);

Ditto regarding maxMatchCount.

> Source/WebKit2/UIProcess/API/mac/WKViewInternal.h:75
> +- (void)_findStringInCustomRepresentation:(NSString *)string withFindOptions:(NSUInteger)options maxMatchCount:(NSUInteger)count;
> +- (void)_countStringMatchesInCustomRepresentation:(NSString *)string withFindOptions:(NSUInteger)options maxMatchCount:(NSUInteger)count;

These are internal methods, so the options parameter can be a WebKit::FindOptions.

[Maciej Stachowiak]

Comment on attachment 88849 [details]
Patch

I'm going to say r+ despite all of Dan's comments, because the change looks functionally correct, and much of this is copied code which already had the issues in question. But please take a look at them and consider fixing those issues before landing.

[Jer Noble]

Committed r83454: <http://trac.webkit.org/changeset/83454>