[webkit-reviews] review granted: [Bug 111246] PDFPlugin: Hook up Services : [Attachment 191120] delete some duplicated code now that we can use pluginViewForFrame in WebPageMac

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 111246: PDFPlugin: Hook up Services
https://bugs.webkit.org/show_bug.cgi?id=111246

Attachment 191120: delete some duplicated code now that we can use
pluginViewForFrame in WebPageMac
https://bugs.webkit.org/attachment.cgi?id=191120&action=review

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


r=me assuming that you tested how this affects Flash.

> Source/WebKit2/WebProcess/Plugins/Plugin.h:270
> +    virtual String getStringSelection() const = 0;

This function name doesn't look grammatically correct to me. Perhaps "get
string for selection" or "get selection string" would be better?

> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:190
> -    EditorState state = m_page->editorState();
> -
> -    m_page->send(Messages::WebPageProxy::EditorStateChanged(state));
> +    m_page->didChangeSelection();

Should the rest of the function go to Page now? I think that it's best to not
have much logic in WebEditorClient.cpp.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:566
> +	   if (pluginView->getStringSelection() != emptyString()) {

Using a special value for in-band error reporting doesn't seem to cause any
issues here, but it would be nicer to not overload String semantics. If doing
it differently is much harder, perhaps null could be the special value, not
empty?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:662
> +	   if (selection != emptyString()) {
> +	       stringValue = selection;

What is the danger in returning the empty string? Please add a comment to the
code, because it looks surprising.