Summary: | WK2: PDF: Find in page | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||||||
Component: | Assignee: | 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
Jer Noble
2011-04-04 10:47:52 PDT
Created attachment 88084 [details]
Patch
Created attachment 88146 [details]
Patch
Re-upload after rebasing; this should fix the buildire's inability to merge.
Attachment 88084 [details] did not build on mac: Build output: http://queues.webkit.org/results/8332125 Whoops, no need for those new functions on PDFViewController to be virtual. Attachment 88146 [details] did not build on qt: Build output: http://queues.webkit.org/results/8333096 Created attachment 88147 [details]
Patch
Attachment 88147 [details] did not build on qt: Build output: http://queues.webkit.org/results/8336053 Created attachment 88151 [details]
Patch
Attachment 88151 [details] did not build on win: Build output: http://queues.webkit.org/results/8334127 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? (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. Created attachment 88747 [details]
Patch
Added stubs to the windows PageClient.h subclass.
Attachment 88747 [details] did not build on qt: Build output: http://queues.webkit.org/results/8342721 Attachment 88747 [details] did not build on win: Build output: http://queues.webkit.org/results/8373365 Attachment 88747 [details] did not build on mac: Build output: http://queues.webkit.org/results/8374035 Drat, part of my patch got merged out of existence. I'll re-upload. Created attachment 88829 [details]
Patch
Attachment 88829 [details] did not build on qt: Build output: http://queues.webkit.org/results/8377062 Somehow managed to merge my Qt changes out of existence as well. :-/ Attachment 88829 [details] did not build on win: Build output: http://queues.webkit.org/results/8377063 ::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. Attachment 88829 [details] did not build on mac: Build output: http://queues.webkit.org/results/8374175 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. 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.
Committed r83454: <http://trac.webkit.org/changeset/83454> |