[webkit-reviews] review granted: [Bug 54777] Mac OS X Services are not available for selected text in WebKit2 windows : [Attachment 83028] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 54777: Mac OS X Services are not available for selected text in WebKit2
windows
https://bugs.webkit.org/show_bug.cgi?id=54777

Attachment 83028: Patch
https://bugs.webkit.org/attachment.cgi?id=83028&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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(pasteboardNam
e, 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?