RESOLVED FIXED 57765
WK2: PDF: Find in page
https://bugs.webkit.org/show_bug.cgi?id=57765
Summary WK2: PDF: Find in page
Jer Noble
Reported 2011-04-04 10:47:52 PDT
We don't support find in page when displaying PDFs
Attachments
Patch (16.58 KB, patch)
2011-04-04 11:17 PDT, Jer Noble
no flags
Patch (16.58 KB, patch)
2011-04-04 15:52 PDT, Jer Noble
no flags
Patch (16.56 KB, patch)
2011-04-04 16:07 PDT, Jer Noble
no flags
Patch (17.50 KB, patch)
2011-04-04 16:22 PDT, Jer Noble
no flags
Patch (17.39 KB, patch)
2011-04-07 18:13 PDT, Jer Noble
no flags
Patch (18.10 KB, patch)
2011-04-08 09:34 PDT, Jer Noble
no flags
Patch (18.79 KB, patch)
2011-04-08 11:43 PDT, Jer Noble
mjs: review+
Jer Noble
Comment 1 2011-04-04 10:48:19 PDT
Jer Noble
Comment 2 2011-04-04 11:17:36 PDT
Jer Noble
Comment 3 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.
WebKit Review Bot
Comment 4 2011-04-04 15:54:02 PDT
Jer Noble
Comment 5 2011-04-04 16:05:16 PDT
Whoops, no need for those new functions on PDFViewController to be virtual.
Early Warning System Bot
Comment 6 2011-04-04 16:06:03 PDT
Jer Noble
Comment 7 2011-04-04 16:07:00 PDT
Early Warning System Bot
Comment 8 2011-04-04 16:20:19 PDT
Jer Noble
Comment 9 2011-04-04 16:22:46 PDT
Build Bot
Comment 10 2011-04-04 17:51:21 PDT
Maciej Stachowiak
Comment 11 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?
Jer Noble
Comment 12 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.
Jer Noble
Comment 13 2011-04-07 18:13:04 PDT
Created attachment 88747 [details] Patch Added stubs to the windows PageClient.h subclass.
Early Warning System Bot
Comment 14 2011-04-07 18:33:00 PDT
Build Bot
Comment 15 2011-04-07 20:48:56 PDT
Eric Seidel (no email)
Comment 16 2011-04-08 00:36:29 PDT
Jer Noble
Comment 17 2011-04-08 09:24:49 PDT
Drat, part of my patch got merged out of existence. I'll re-upload.
Jer Noble
Comment 18 2011-04-08 09:34:35 PDT
Early Warning System Bot
Comment 19 2011-04-08 09:43:50 PDT
Jer Noble
Comment 20 2011-04-08 09:55:29 PDT
Somehow managed to merge my Qt changes out of existence as well. :-/
Build Bot
Comment 21 2011-04-08 10:02:45 PDT
Jer Noble
Comment 22 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.
WebKit Review Bot
Comment 23 2011-04-08 10:42:51 PDT
Jer Noble
Comment 24 2011-04-08 11:43:01 PDT
mitz
Comment 25 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.
Maciej Stachowiak
Comment 26 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.
Jer Noble
Comment 27 2011-04-11 11:11:03 PDT
Note You need to log in before you can comment on or make changes to this bug.