Bug 138032 – WebKit1 should support action menus

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Beth Dakin]

WebKit1 should support action menus.

[Beth Dakin]

Created attachment 240381 [details]
Patch

[WebKit Commit Bot]

Attachment 240381 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebActionMenuHelper.h:31:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebView/WebActionMenuHelper.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

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.

> Source/WebKit/mac/WebView/WebActionMenuHelper.h:31
> +@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.

[Beth Dakin]

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

[Beth Dakin]

Created attachment 240423 [details]
Patch that addresses Dan's comments

[WebKit Commit Bot]

Attachment 240423 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebActionMenuController.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 240381 [details]
Patch

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

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

My point is more that there are no hard guarantees about object lifetimes in Objective-C. Even if the WebViewData is released, and even if that release causes it to be deallocated, and that causes the helper to be released, it’s still possible that the helper won’t be deallocated immediately, because some other object got a strong reference to it, or it found its way into an autorelease pool. In practice, perhaps this won’t matter because at least we know that once the WebView is gone, nothing can message the helper and cause it to do anything that references the WebView. If we weren’t (or aren’t) sure of that, we could do something safer and have an explicit way for the WebView’s -dealloc (or -close) to tell the helper that it’s going away.

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

No need to forward declare, just NSClassFromString() it!

[Beth Dakin]

Created attachment 240435 [details]
Patch

[WebKit Commit Bot]

Attachment 240435 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebActionMenuController.mm:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 240435 [details]
Patch

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

> Source/WebKit/mac/WebView/WebActionMenuController.mm:82
> +    NSDictionary *hitTestResult = [_webView elementAtPoint:[_webView convertPoint:[event locationInWindow] fromView:nil]];

event.locationInWindow

> Source/WebKit/mac/WebView/WebActionMenuController.mm:96
> +    NSURL *url = [sender representedObject];

dots.

[Beth Dakin]

http://trac.webkit.org/changeset/175182