Bug 57527 – WebKit2: No "open in preview" contextual menu item for PDFs

• Component: PDF
• Status: RESOLVED FIXED

[Jer Noble]

PDFs cannot be opened in Preview through the context menu in WebKit2.

[Jer Noble]

<rdar://problem/9210368>

[Jer Noble]

Created attachment 87674 [details]
Patch

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!

[Jer Noble]

(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...

[Jer Noble]

Created attachment 87678 [details]
Patch

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

[Jer Noble]

(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!

[Jer Noble]

(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.

[Jer Noble]

Committed r82852: <http://trac.webkit.org/changeset/82852>