Bug 57527

Summary: WebKit2: No "open in preview" contextual menu item for PDFs
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: PDFAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal Keywords: InRadar, PlatformOnly
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
URL: http://ne.edgecastcdn.net/800E0A/www.smartusa.com/Downloads/smart-fortwo-electric-drive.pdf
Attachments:
Description Flags
Patch
none
Patch mitz: review+

Description Jer Noble 2011-03-30 23:56:21 PDT 
PDFs cannot be opened in Preview through the context menu in WebKit2.
Comment 1 Jer Noble 2011-03-30 23:56:51 PDT 
<rdar://problem/9210368>
Comment 2 Jer Noble 2011-03-31 00:03:49 PDT 
Created attachment 87674 [details]
Patch
Comment 3 mitz 2011-03-31 00:29:54 PDT 
Comment on attachment 87674 [details]
Patch

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

Too many comments to r+. I suspect that this is leaking menu items.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:66
> +    CFRelease (appURL);

Extra space after CFRelease. Given that you’re using this variable twice as a CFURLRef an only once as an NSURL *, I would define it as a CFURLRef and cast once.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:69
> +    [*image setSize:NSMakeSize(16.f,16.f)];

Should be NSMakeSize(16, 16) without .f and with a space after the comma.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:71
> +    NSString *appName = [[NSFileManager defaultManager] displayNameAtPath:appPath];

Not sure why this local is needed.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:224
> +    NSMenu* menu = [[NSMenu alloc] initWithTitle:@""];

Space on the wrong side of the *.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:226
> +    NSEnumerator *e = [[[_pdfView menuForEvent:theEvent] itemArray] objectEnumerator];

Please use a more descriptive name.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:228
> +    while ((item = [e nextObject]) != nil) {

WebKit style is to omit “!= nil”

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:230
> +        NSMenuItem *itemCopy = [item copy];
> +        [menu addItem:itemCopy];

Wow, we don’t need to release the item?

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:242
> +        if (!appName)
> +            // FIXME: Localize this.
> +            appName = @"Finder";

Need braces around these two lines.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:252
> +        [menu addItem:item];

Don’t need to release item?

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:265
> +    return YES;

Strange that we copy the PDFView’s own items then decide for it that they’re always valid. But that’s what we do in WebKit1!
Comment 4 Jer Noble 2011-03-31 01:13:19 PDT 
(In reply to comment #3)
> (From update of attachment 87674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87674&action=review
> 
> Too many comments to r+. I suspect that this is leaking menu items.

You're right, I didn't check this nearly well enough when copying it from WebKit1.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:66
>> +    CFRelease (appURL);
> 
> Extra space after CFRelease. Given that you’re using this variable twice as a CFURLRef an only once as an NSURL *, I would define it as a CFURLRef and cast once.

Absolutely.  All changed.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:69
>> +    [*image setSize:NSMakeSize(16.f,16.f)];
> 
> Should be NSMakeSize(16, 16) without .f and with a space after the comma.

Changed.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:71
>> +    NSString *appName = [[NSFileManager defaultManager] displayNameAtPath:appPath];
> 
> Not sure why this local is needed.

Neither am I.   Collapsed.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:224
>> +    NSMenu* menu = [[NSMenu alloc] initWithTitle:@""];
> 
> Space on the wrong side of the *.

Indeed.  Swapped.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:226
>> +    NSEnumerator *e = [[[_pdfView menuForEvent:theEvent] itemArray] objectEnumerator];
> 
> Please use a more descriptive name.

Sure thing.  "menuItemEnumerator".

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:228
>> +    while ((item = [e nextObject]) != nil) {
> 
> WebKit style is to omit “!= nil”

I think, in addition, the declaration can probably be pulled into the while conditional as well.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:230
>> +        [menu addItem:itemCopy];
> 
> Wow, we don’t need to release the item?

Totally untrue.  We really do need to release the item.  Good catch.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:242
>> +            appName = @"Finder";
> 
> Need braces around these two lines.

This was definitely my fault.  Braced.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:252
>> +        [menu addItem:item];
> 
> Don’t need to release item?

I think we do.  It was only above that we were doing the wrong thing.

>> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:265
>> +    return YES;
> 
> Strange that we copy the PDFView’s own items then decide for it that they’re always valid. But that’s what we do in WebKit1!

I think that the way this will work is: we don't change any of the copied menu items' targets, which are presumably the PDFView.  So it will still get called on to validate those menu items.

Posting a new patch shortly...
Comment 5 Jer Noble 2011-03-31 01:20:47 PDT 
Created attachment 87678 [details]
Patch
Comment 6 mitz 2011-04-03 16:21:15 PDT 
Comment on attachment 87678 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:62
> +    CFURLRef appURL = nil;

Now that it’s a CF type, it should be = 0, not = nil.

> Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:68
> +    NSString *appPath = [(NSURL*)appURL path];

Need space after NSURL
Comment 7 Jer Noble 2011-04-04 10:57:00 PDT 
(In reply to comment #6)
> (From update of attachment 87678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87678&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:62
> > +    CFURLRef appURL = nil;
> 
> Now that it’s a CF type, it should be = 0, not = nil.

Actually, since CFURLRef is a C-style opaque pointer, shouldn't it be NULL?  

> > Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:68
> > +    NSString *appPath = [(NSURL*)appURL path];
> 
> Need space after NSURL

Added.  Thanks!
Comment 8 Jer Noble 2011-04-04 11:03:10 PDT 
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 87678 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87678&action=review
> > 
> > > Source/WebKit2/UIProcess/API/mac/PDFViewController.mm:62
> > > +    CFURLRef appURL = nil;
> > 
> > Now that it’s a CF type, it should be = 0, not = nil.
> 
> Actually, since CFURLRef is a C-style opaque pointer, shouldn't it be NULL?  

Looks like I'm confusing the language of the file with the language of the pointer.  Changed.
Comment 9 Jer Noble 2011-04-04 12:19:50 PDT 
Committed r82852: <http://trac.webkit.org/changeset/82852>