Bug 138034 – [Mac][WebKit2] Move action menu code into its own file

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

[Mac][WebKit2] Move action menu code into its own file

[Tim Horton]

Created attachment 240382 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/18758758>

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:225:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 5 files


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

Comment on attachment 240382 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:61
> +using namespace WebCore;
> +using namespace WebKit;

I thought using namespace {WebCore, WebKit} had fallen out of favor lately.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:71
> +@interface WKActionMenuController (Internal)
> +- (void)_updateActionMenuItems;
> +@end

Why does this need to be in a category?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:75
> +    WebPageProxy *_page;
> +    WKView *_wkView;

How do we ensure that we don’t access these ivars beyond these objects’ lifetimes?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:94
> +    _wkView.actionMenu = _menu.get();

It’s a little strange for the initializer to mutate the view like this.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:106
> +    _page->performActionMenuHitTestAtLocation([_wkView convertPoint:[event locationInWindow] fromView:nil]);

event.locationInWindow

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:148
> +@end
> +
> +@implementation WKActionMenuController (Internal)

Why is this category needed?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:155
> +    if (!hitTestResult)

Why do we check this if our only caller already does?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:166
> +    return @[openLinkItem.get(), previewLinkItem.get(), [NSMenuItem separatorItem], readingListItem.get()];

Please add spaces after @[ and before ].

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:176
> +    NSURL *url = [NSURL URLWithString:hitTestResult->absoluteLinkURL()];
> +    [[NSWorkspace sharedWorkspace] openURL:url];

Can you use -_web_URLWithWTFString: instead of URLWithString: here? No need for the local url variable.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:187
> +    NSURL *url = [NSURL URLWithString:hitTestResult->absoluteLinkURL()];
> +    NSSharingService *service = [NSSharingService sharingServiceNamed:NSSharingServiceNameAddToSafariReadingList];
> +    [service performWithItems:@[ url ]];

Can you use -_web_URLWithWTFString:? No need for so many local variables here.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:209
> +    [bubble showPreviewItem:[NSURL URLWithString:hitTestResult->absoluteLinkURL()] itemFrame:itemFrame];

Can you use -_web_URLWithWTFString: here?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:217
> +    if (!hitTestResult)

Why do we check this if our only caller already does?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:232
> +    return @[copyImageItem.get(), addToPhotosItem.get(), saveToDownloadsItem.get(), shareItem.get()];

Please add spaces after @[ and before ].

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:257
> +    _page->process().context().download(_page, URL(URL(), hitTestResult->absoluteImageURL()));

We can’t trust absoluteImageURL() to be already-parsed?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:302
> +static NSString *temporaryPhotosDirectoryPath()
> +{
> +    static NSString *temporaryPhotosDirectoryPath;
> +
> +    if (!temporaryPhotosDirectoryPath) {
> +        NSString *temporaryDirectoryTemplate = [NSTemporaryDirectory() stringByAppendingPathComponent:@"WebKitPhotos-XXXXXX"];
> +        CString templateRepresentation = [temporaryDirectoryTemplate fileSystemRepresentation];
> +
> +        if (mkdtemp(templateRepresentation.mutableData()))
> +            temporaryPhotosDirectoryPath = [[[NSFileManager defaultManager] stringWithFileSystemRepresentation:templateRepresentation.data() length:templateRepresentation.length()] copy];
> +    }
> +
> +    return temporaryPhotosDirectoryPath;
> +}
> +
> +static NSString *pathToPhotoOnDisk(NSString *suggestedFilename)
> +{
> +    NSString *photoDirectoryPath = temporaryPhotosDirectoryPath();
> +    if (!photoDirectoryPath) {
> +        WTFLogAlways("Cannot create temporary photo download directory.");
> +        return nil;
> +    }
> +
> +    NSString *path = [photoDirectoryPath stringByAppendingPathComponent:suggestedFilename];
> +
> +    NSFileManager *fileManager = [NSFileManager defaultManager];
> +    if ([fileManager fileExistsAtPath:path]) {
> +        NSString *pathTemplatePrefix = [photoDirectoryPath stringByAppendingPathComponent:@"XXXXXX-"];
> +        NSString *pathTemplate = [pathTemplatePrefix stringByAppendingString:suggestedFilename];
> +        CString pathTemplateRepresentation = [pathTemplate fileSystemRepresentation];
> +
> +        int fd = mkstemps(pathTemplateRepresentation.mutableData(), pathTemplateRepresentation.length() - strlen([pathTemplatePrefix fileSystemRepresentation]) + 1);
> +        if (fd < 0) {
> +            WTFLogAlways("Cannot create photo file in the temporary directory (%@).", suggestedFilename);
> +            return nil;
> +        }
> +
> +        close(fd);
> +        path = [fileManager stringWithFileSystemRepresentation:pathTemplateRepresentation.data() length:pathTemplateRepresentation.length()];
> +    }
> +
> +    return path;
> +}

This is almost identical to PDF-related functions in WebPageProxyMac.mm. You should refactor and reuse that code.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:306
> +    // FIXME: We shouldn't even add the button if this is the case, for now.

s/button/menu item/

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:307
> +    if (![getIKSlideshowClass() canExportToApplication:(@"com.apple.Photos")])

Why the parentheses around the NSString literal?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:333
> +            [getIKSlideshowClass() exportSlideshowItem:filePath toApplication:(@"com.apple.Photos")];

Why the parentheses around the NSString literal?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:342
> +    SEL selector = nil;

SEL isn’t an id, so you should use 0 here.

Comment on attachment 240382 [details]
Patch

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

>> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:342
>> +    SEL selector = nil;
> 
> SEL isn’t an id, so you should use 0 here.

Or nullptr.

[Tim Horton]

http://trac.webkit.org/changeset/175178 and http://trac.webkit.org/changeset/175179

[Tim Horton]

And https://trac.webkit.org/changeset/175180

[Tim Horton]

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

[Tim Horton]

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

[Tim Horton]

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