[webkit-reviews] review granted: [Bug 56975] WebKit2:Services menu item to convert selected Simplified/Traditional Chinese Text is not working : [Attachment 86822] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 56975: WebKit2:Services menu item to convert selected
Simplified/Traditional Chinese Text is not working
https://bugs.webkit.org/show_bug.cgi?id=56975

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86822&action=review

> Source/WebCore/editing/mac/EditorMac.mm:219
> +void Editor::readSelectionFromPasteboard(const String& pasteboardName)

Should this be share with WebKit1?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:503
> +// The following three methods are needed to support Mac OS X services.

I would have put a "This method is needed to support Mac OS X Services" before
each of the two (?) functions that are required. It's easy to overlook a
comment that's position like this.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:540
> +    /*
> +    Frame* coreFrame = core([self _frame]); 
> +    if (!coreFrame) 
> +	   return NO; 
> +    if (coreFrame->selection()->isContentRichlyEditable()) 
> +	   [self _pasteWithPasteboard:pasteboard allowPlainText:YES]; 
> +    else 
> +	   [self _pasteAsPlainTextWithPasteboard:pasteboard]; 
> +	*/

Commented out code.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:167
> +    const double MessageTimeout = 20;

Why capital M? 

I assume that this is in seconds. Seems appropriate to me, but it's strange to
see the constant in the function, not at the top of the file.

> LayoutTests/ChangeLog:5
> +	   Repeated copy and paste-in-place operation results in increasingly
verbose HTML.

This is from another patch.