[webkit-reviews] review denied: [Bug 57527] WebKit2: No "open in preview" contextual menu item for PDFs : [Attachment 87674] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Jer Noble <jer.noble at apple.com>'s request for
review:
Bug 57527: WebKit2: No "open in preview" contextual menu item for PDFs
https://bugs.webkit.org/show_bug.cgi?id=57527

Attachment 87674: Patch
https://bugs.webkit.org/attachment.cgi?id=87674&action=review

------- Additional Comments from mitz at webkit.org
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!