Bug 111246 – PDFPlugin: Hook up Services

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

OS X Services, that is.

<rdar://problem/13062672>

[Tim Horton]

Created attachment 191099 [details]
preliminary patch

Missing a changelog and support for services that consume rich text data (e.g. New Email with Selection). Not sure how to best implement that yet.

[Tim Horton]

Created attachment 191119 [details]
patch

[Tim Horton]

Created attachment 191120 [details]
delete some duplicated code now that we can use pluginViewForFrame in WebPageMac

[Alexey Proskuryakov]

Comment on attachment 191120 [details]
delete some duplicated code now that we can use pluginViewForFrame in WebPageMac

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.

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 191120 [details])
> 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?

I thought the same thing, but this is the name that Pasteboard and WebPage already use...

> > 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.

Don't see why not.

> > 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?

Probably a good plan.

> 
> > 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.

No danger, I was just trying to keep the behavior precisely the same in the case where the plugin doesn't support getStringSelection() (i.e. all non-PDFPlugin plugins).

[Tim Horton]

(In reply to comment #5)
> > > 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.
> 
> Don't see why not.

To clarify, you just meant the call to updateGlobalSelection (and the implementation of updateGlobalSelection), right? Not the InjectedBundleEditorClient notification.

[Tim Horton]

Created attachment 191165 [details]
patch

Still some things to discuss with Alexey, but I renamed getStringSelection -> getSelectionString (seems more important to have a good name here than match the rest of the code) and switched to using a null string for the "there is no selection" case.

[Alexey Proskuryakov]

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

> To clarify, you just meant the call to updateGlobalSelection (and the implementation of updateGlobalSelection), right? Not the InjectedBundleEditorClient notification.

Honestly, I did not look at what else the function did, so this was truly a question.

[Tim Horton]

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