Bug 54777 – Mac OS X Services are not available for selected text in WebKit2 windows

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

* STEPS TO REPRODUCE
1. In a WebKit2 window, navigate to <http://www.apple.com/>
2. Select a word on the page
3. Choose Safari > Services

* RESULTS
A menu item saying “No Services Apply”.

[Enrica Casucci]

Created attachment 83028 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/Pasteboard.h:89:  The parameter name "pasteboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:89:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 19 files


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

[Enrica Casucci]

I've fixed the style issue.

[Adam Roben (:aroben)]

Comment on attachment 83028 [details]
Patch

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

> Source/WebCore/editing/mac/EditorMac.mm:213
> +    RetainPtr<NSMutableArray> types = [NSMutableArray array];

It would be a little more efficient to use the AdoptNS constructor and +alloc/-init rather than +array. That would also match our general style of only using autoreleased objects when absolutely necessary.

> Source/WebCore/editing/mac/EditorMac.mm:216
> +    Pasteboard::writeSelection([NSPasteboard pasteboardWithName:pasteboardName], (NSArray*)types.get(), selectedRange().get(), true, m_frame);

I'm surprised the cast is needed here.

> Source/WebCore/platform/mac/PasteboardMac.mm:156
> +    NSArray *types = (!pasteboardTypes) ? frame->editor()->client()->pasteboardTypesForSelection(frame) : pasteboardTypes;

No need for the parentheses here. I think this would be a little clearer if you got rid of the !, too: pasteboardTypes ? pasteboardTypes : ...

> Source/WebCore/platform/mac/PasteboardMac.mm:166
> +    NSArray *types = (!pasteboardTypes) ? selectionPasteboardTypes(canSmartCopyOrDelete, [attributedString containsAttachments]) : pasteboardTypes;

Same comments as above.

> Source/WebKit2/Shared/mac/PasteboardTypes.mm:74
> +    static NSArray *types = retain([NSArray arrayWithObjects:WebArchivePboardType, NSRTFDPboardType, NSRTFPboardType, NSStringPboardType, nil]);

If you used +alloc/-initWithObjects: and removed the retain() call you would avoid an unnecessary -autorelease/CFRetain.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:653
> +    process()->sendSync(Messages::WebPage::WriteSelectionToPasteboard(pasteboardName, pasteboardTypes), Messages::WebPage::WriteSelectionToPasteboard::Reply(result), m_pageID, 20);

What is the "20" here? Is it a timeout value? How was it chosen? Putting it in a constant with a comment explaining why this particular value is used would be better.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:211
>  
> +    

Extra blank line here.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:213
> +    [NSApp registerServicesMenuSendTypes:PasteboardTypes::forSelection() 
> +                             returnTypes:PasteboardTypes::forEditing()];

I think this could all go on one line.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:506
> +    for (size_t i = 0; i < [types count]; ++i)

You should store the result of -count in a local variable so it isn't called on each loop iteration.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:511
> +- (id)validRequestorForSendType:(NSString *)sendType returnType:(NSString *)returnType

This method might be clearer if you were to add two helper functions: one for validating the send type, and one for validating the return type.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:514
> +    BOOL isSendTypeOK = !sendType || ([PasteboardTypes::forSelection() containsObject:sendType] && !_data->_page->selectionState().isNone);
> +    BOOL isReturnTypeOK = NO;

I think isValidSendType and isValidReturnType would be slightly clearer than these names.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:519
> +    else if ([PasteboardTypes::forEditing() containsObject:returnType] && _data->_page->selectionState().isContentEditable) {
> +        // We can insert strings in any editable context.  We can insert other types, like images, only in rich edit contexts.
> +        isReturnTypeOK = [returnType isEqualToString:NSStringPboardType] || _data->_page->selectionState().isContentRichlyEditable;

Swapping the sides of these && expressions could be more efficient, since you could avoid the ObjC method dispatch in some cases.

> Source/WebKit/mac/WebView/WebHTMLView.mm:2613
> +    NSLog(@"Pasteboard name: %@", [pasteboard name]);

I assume you meant to remove this?

[Enrica Casucci]

Thanks for the review!
I've addressed your comments.

[Enrica Casucci]

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