[Webkit-unassigned] [Bug 138032] WebKit1 should support action menus

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=138032

--- Comment #3 from mitz at webkit.org <mitz at webkit.org> ---
Comment on attachment 240381
  --> https://bugs.webkit.org/attachment.cgi?id=240381
Patch

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

> Source/WebKit/mac/WebView/WebActionMenuHelper.h:26
> +#import <Foundation/Foundation.h>

This shouldn’t be necessary thanks to the prefix header.

> Source/WebKit/mac/WebView/WebActionMenuHelper.h:31
> + at interface WebActionMenuHelper : NSObject
> +{

More often than not, we put the curly brace on the same line as @interface. You should also add a @private line before the ivar declaration.

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:32
> +

No need for this newline

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:62
> +    _webView = webView;

Like I asked Tim in the other patch: what keeps us from using this ivar after the WebView is deallocated?

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:69
> +    if (menu != [_webView actionMenu])

_webView.actionMenu

Maybe we should keep it in a local variable here so that we don’t call -actionMenu three times.

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:86
> +    if (url == nil)

if (!url)

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:88
> +

Perhaps we should assert that url is really an NSURL.

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:95
> +    if (url == nil)

Ditto

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:104
> +    if (domElement == nil)

!domElement

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:121
> +    NSDictionary *hitTestResult = [sender representedObject];

Perhaps we should assert that this is really an NSDictionary (or nil).

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:123
> +    if (url == nil)

!url

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:127
> +    if (domElement == nil)

!domElement

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:134
> +    QuickLookUILibrary();
> +    RetainPtr<QLPreviewBubble> bubble = adoptNS([[NSClassFromString(@"QLPreviewBubble") alloc] init]);

Instead of NSClassFromString(…), you should use getQLPreviewBubbleClass(), which takes care of loading the library, and remove the explicit QuickLookUILibrary.

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:149
> +    SEL selector = nil;

SEL is not an id, so this should be nullptr, not nil.

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:191
> +    return [[NSBundle bundleForClass:[WebView class]] imageForResource:name];

If you changed this to [WKView class], then it would look in the WebKit.framework bundle (rather than the WebKitLegacy.framework bundle) and you wouldn’t need to add the image to this project at all!

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:197
> +    if (url == nil)

!url

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:207
> +    return @[openLinkItem.get(), previewLinkItem.get(), [NSMenuItem separatorItem], readingListItem.get()];

Please add spaces after @[ and before ].

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:213
> +    if (url != nil)

!url

> Source/WebKit/mac/WebView/WebActionMenuHelper.mm:216
> +    return @[];

@[       ]

> Source/WebKit/mac/WebView/WebView.mm:1070
> +        [self setActionMenu:actionMenu.get()];

self.actionMenu = …

> Source/WebKit/mac/WebView/WebView.mm:1071
> +        _private->actionMenuHelper = [[[WebActionMenuHelper alloc] initWithWebView:self] retain];

This is an over-retain, because +alloc already gives you ownership of the object.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141024/f72efb9d/attachment-0002.html>