Bug 57765

Summary: WK2: PDF: Find in page
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: PDFAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, eric, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch mjs: review+

Description Jer Noble 2011-04-04 10:47:52 PDT
We don't support find in page when displaying PDFs
Comment 1 Jer Noble 2011-04-04 10:48:19 PDT
<rdar://problem/8750336>
Comment 2 Jer Noble 2011-04-04 11:17:36 PDT
Created attachment 88084 [details]
Patch
Comment 3 Jer Noble 2011-04-04 15:52:21 PDT
Created attachment 88146 [details]
Patch

Re-upload after rebasing; this should fix the buildire's inability to merge.
Comment 4 WebKit Review Bot 2011-04-04 15:54:02 PDT
Attachment 88084 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8332125
Comment 5 Jer Noble 2011-04-04 16:05:16 PDT
Whoops, no need for those new functions on PDFViewController to be virtual.
Comment 6 Early Warning System Bot 2011-04-04 16:06:03 PDT
Attachment 88146 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8333096
Comment 7 Jer Noble 2011-04-04 16:07:00 PDT
Created attachment 88147 [details]
Patch
Comment 8 Early Warning System Bot 2011-04-04 16:20:19 PDT
Attachment 88147 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8336053
Comment 9 Jer Noble 2011-04-04 16:22:46 PDT
Created attachment 88151 [details]
Patch
Comment 10 Build Bot 2011-04-04 17:51:21 PDT
Attachment 88151 [details] did not build on win:
Build output: http://queues.webkit.org/results/8334127
Comment 11 Maciej Stachowiak 2011-04-06 23:59:35 PDT
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?
Comment 12 Jer Noble 2011-04-07 07:54:10 PDT
(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.
Comment 13 Jer Noble 2011-04-07 18:13:04 PDT
Created attachment 88747 [details]
Patch

Added stubs to the windows PageClient.h subclass.
Comment 14 Early Warning System Bot 2011-04-07 18:33:00 PDT
Attachment 88747 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8342721
Comment 15 Build Bot 2011-04-07 20:48:56 PDT
Attachment 88747 [details] did not build on win:
Build output: http://queues.webkit.org/results/8373365
Comment 16 Eric Seidel (no email) 2011-04-08 00:36:29 PDT
Attachment 88747 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8374035
Comment 17 Jer Noble 2011-04-08 09:24:49 PDT
Drat, part of my patch got merged out of existence.  I'll re-upload.
Comment 18 Jer Noble 2011-04-08 09:34:35 PDT
Created attachment 88829 [details]
Patch
Comment 19 Early Warning System Bot 2011-04-08 09:43:50 PDT
Attachment 88829 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8377062
Comment 20 Jer Noble 2011-04-08 09:55:29 PDT
Somehow managed to merge my Qt changes out of existence as well. :-/
Comment 21 Build Bot 2011-04-08 10:02:45 PDT
Attachment 88829 [details] did not build on win:
Build output: http://queues.webkit.org/results/8377063
Comment 22 Jer Noble 2011-04-08 10:18:54 PDT
::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.
Comment 23 WebKit Review Bot 2011-04-08 10:42:51 PDT
Attachment 88829 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8374175
Comment 24 Jer Noble 2011-04-08 11:43:01 PDT
Created attachment 88849 [details]
Patch
Comment 25 mitz 2011-04-10 17:16:49 PDT
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.
Comment 26 Maciej Stachowiak 2011-04-10 18:41:57 PDT
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.
Comment 27 Jer Noble 2011-04-11 11:11:03 PDT
Committed r83454: <http://trac.webkit.org/changeset/83454>