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

[bugzilla-daemon at webkit.org]

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

--- Comment #4 from Beth Dakin <bdakin at apple.com> ---
Thank you for your comments, Dan!

(In reply to comment #3)
> Comment on attachment 240381 [details]
> 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.
> 

Good to know! Fixed.

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

Fixed.

> > Source/WebKit/mac/WebView/WebActionMenuHelper.mm:32
> > +
> 
> No need for this newline
> 

Fixed.

> > 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?
> 

Hopefully this whole class will be deallocated while the WebView is being deallocated. WebViewData has the reference to the WebActionMenuHelper, and the WebView releases the WebViewData in its dealloc, and the WebViewData releases the WebActionMenuHelper in its dealloc. So I think we're okay, but please correct me if I am wrong.

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

Fixed.

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

Fixed.

> > Source/WebKit/mac/WebView/WebActionMenuHelper.mm:88
> > +
> 
> Perhaps we should assert that url is really an NSURL.
> 

Fixed.

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

Fixed.

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

Fixed.

> > Source/WebKit/mac/WebView/WebActionMenuHelper.mm:121
> > +    NSDictionary *hitTestResult = [sender representedObject];
> 
> Perhaps we should assert that this is really an NSDictionary (or nil).
> 

Fixed.

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

Fixed.

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

Fixed.

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

Oh, yay! Fixed.

> > Source/WebKit/mac/WebView/WebActionMenuHelper.mm:149
> > +    SEL selector = nil;
> 
> SEL is not an id, so this should be nullptr, not nil.
> 

Fixed.

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

WebKit did not want to build with this because it didn't know what a WKView does. Should I do something to forward declare it? Is that okay?

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed!

-- 
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/32f82f8d/attachment-0002.html>