[webkit-reviews] review granted: [Bug 185401] The PDF context menu should not be created in the WebContent process. : [Attachment 339855] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 185401: The PDF context menu should not be created in the WebContent
process.
https://bugs.webkit.org/show_bug.cgi?id=185401

Attachment 339855: Patch

https://bugs.webkit.org/attachment.cgi?id=339855&action=review




--- Comment #14 from Tim Horton <thorton at apple.com> ---
Comment on attachment 339855
  --> https://bugs.webkit.org/attachment.cgi?id=339855
Patch

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

> Source/WebKit/Shared/mac/PDFContextMenu.h:32
> +    String m_title;

Public fields in structs don’t usually get m_ prefixes, right?

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:79
> +    id menuItem;

should this be “selectedMenuItem”? Also, ObjC members get a leading underscore.

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:81
> +- (instancetype)init;

This is implicit since you’re an NSObject.

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:89
> +    self = [super init];

if (!self)
    return nil;

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:570
> +    auto menuTarget = [[[WKPDFMenuTarget alloc] init] autorelease];
> +    auto nsMenu = [[[NSMenu alloc] init] autorelease];

RetainPtr & adoptNS are the preferred way to solve this problem

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:580
> +	   auto nsItem = [[NSMenuItem alloc] init];

This one is a leak too?

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:591
> +    NSWindow* window = m_pageClient.platformWindow();

ObjC stars go on the other side