WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(16.58 KB, patch)
2011-04-04 15:52 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(16.56 KB, patch)
2011-04-04 16:07 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(17.50 KB, patch)
2011-04-04 16:22 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(17.39 KB, patch)
2011-04-07 18:13 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(18.10 KB, patch)
2011-04-08 09:34 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
Patch
(18.79 KB, patch)
2011-04-08 11:43 PDT
,
Jer Noble
mjs
: review+
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Jer Noble
Comment 1
2011-04-04 10:48:19 PDT
<
rdar://problem/8750336
>
Jer Noble
Comment 2
2011-04-04 11:17:36 PDT
Created
attachment 88084
[details]
Patch
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
Attachment 88084
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8332125
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
Attachment 88146
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8333096
Jer Noble
Comment 7
2011-04-04 16:07:00 PDT
Created
attachment 88147
[details]
Patch
Early Warning System Bot
Comment 8
2011-04-04 16:20:19 PDT
Attachment 88147
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8336053
Jer Noble
Comment 9
2011-04-04 16:22:46 PDT
Created
attachment 88151
[details]
Patch
Build Bot
Comment 10
2011-04-04 17:51:21 PDT
Attachment 88151
[details]
did not build on win: Build output:
http://queues.webkit.org/results/8334127
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
Attachment 88747
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8342721
Build Bot
Comment 15
2011-04-07 20:48:56 PDT
Attachment 88747
[details]
did not build on win: Build output:
http://queues.webkit.org/results/8373365
Eric Seidel (no email)
Comment 16
2011-04-08 00:36:29 PDT
Attachment 88747
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8374035
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
Created
attachment 88829
[details]
Patch
Early Warning System Bot
Comment 19
2011-04-08 09:43:50 PDT
Attachment 88829
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8377062
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
Attachment 88829
[details]
did not build on win: Build output:
http://queues.webkit.org/results/8377063
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
Attachment 88829
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/8374175
Jer Noble
Comment 24
2011-04-08 11:43:01 PDT
Created
attachment 88849
[details]
Patch
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
Committed
r83454
: <
http://trac.webkit.org/changeset/83454
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug