Bug 41649 – [chromium] Add a few more methods to WebPlugin so that Pepper v2 plugins can support copy/zoom/find

• Component: New Bugs
• Status: RESOLVED FIXED

[John Abd-El-Malek]

[chromium] Add a few more methods to WebPlugin so that Pepper v2 plugins can support copy/zoom/find

[John Abd-El-Malek]

Created attachment 60586 [details]
Patch

[Adam Barth]

Comment on attachment 60586 [details]
Patch

Looks plausible, but we need the great and powerful fishd for WebKit API review.

WebKit/chromium/public/WebPlugin.h:101
 +      virtual void zoom(int factor) {}
I think we want { } instead of {}, but I could be wrong.

[John Abd-El-Malek]

Created attachment 60743 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 60743 [details]
Patch

WebKit/chromium/public/WebPlugin.h:99
 +      // Used for zooming of full page plugins.  0 means reset, while -1 means zoom
nit: add a new line above this comment since selectedText and zoom
aren't related methods.

note: WebView::setZoomLevel takes a zoomLevel int and returns the actual int that the
object was zoomed to.  shouldn't WebPlugin be similar?  what about textOnly?  even if
plugins don't support exactly the same zooming capabilities, perhaps the webkit api
shouldn't preclude it?  imagine implementing WebPlugin in terms of WebView.

I assume the "copy" method means that the plugin should copy text to the clipboard,
but how does the plugin do that, and if the selected text can be read from the
WebPlugin, why not have the host simply call selectedText and then take care of the
clipboard operations?

WebKit/chromium/public/WebPlugin.h:105
 +      // See WebPluginContainerImpl's description of the find interface.
i recommend not having public headers refer to private implementation
classes for documentation.  please document the interface here.

WebKit/chromium/public/WebPlugin.h:107
 +      virtual void startFind(const WebString& searchText, bool caseSensitive, int identifier) { }
what is the purpose of the identifier here?  i figure it is
related to the identifier used in the "find" methods on WebFrame,
but what is the plugin supposed to do with the identifier.  are
there some corresponding changes to WebPluginContainer that are
required?  how does the plugin report find-in-page results?

[John Abd-El-Malek]

(In reply to comment #4)
> (From update of attachment 60743 [details])
> WebKit/chromium/public/WebPlugin.h:99
>  +      // Used for zooming of full page plugins.  0 means reset, while -1 means zoom
> nit: add a new line above this comment since selectedText and zoom
> aren't related methods.

done

> 
> note: WebView::setZoomLevel takes a zoomLevel int and returns the actual int that the
> object was zoomed to.  shouldn't WebPlugin be similar?  

For WebView, that's doable because the current zoom is available.  To make the plugin API the same, we'd have to add another function to the plugin to get the current zoom.  That seems unnecessary (adding a getter just so that value can be sent back).

> what about textOnly?  even if
> plugins don't support exactly the same zooming capabilities, perhaps the webkit api
> shouldn't preclude it?  imagine implementing WebPlugin in terms of WebView.

Implementation wise, this code is called from a place in Chrome where the only available information is PageZoom::Function (zoom in/out/100%) so we don't have information on whether this is a text only operation or not.  I can add that value and ignore it, but it seems we can go without it for now and only add it if we need it?

> 
> I assume the "copy" method means that the plugin should copy text to the clipboard,
> but how does the plugin do that, and if the selected text can be read from the
> WebPlugin, why not have the host simply call selectedText and then take care of the
> clipboard operations?

yeah I wanted to do that, but in the implementation I found this easier.  The reason was that in both v1 and v2 implementations, I sniff for "ctrl+c" or "cmd+c" and then call the copy() method directly.  So the plugin will have to know how to use the clipboard anyways.

> 
> WebKit/chromium/public/WebPlugin.h:105
>  +      // See WebPluginContainerImpl's description of the find interface.
> i recommend not having public headers refer to private implementation
> classes for documentation.  please document the interface here.

Done

> 
> WebKit/chromium/public/WebPlugin.h:107
>  +      virtual void startFind(const WebString& searchText, bool caseSensitive, int identifier) { }
> what is the purpose of the identifier here?  i figure it is
> related to the identifier used in the "find" methods on WebFrame,
> but what is the plugin supposed to do with the identifier.  are
> there some corresponding changes to WebPluginContainer that are
> required?  how does the plugin report find-in-page results?

I expanded on this in the comment.  The plugin uses the identifier when talking to WebFrameClient's reportFindInPage* functions.

Are you saying we should just have them talk to WebPluginContainer?  This would be adding more layers that aren't necessary IMO, since the actual implementation (both pepper v1 and v2) don't even use the WebFrameClient directly, but have a RenderView*.

[John Abd-El-Malek]

Created attachment 60795 [details]
Patch

[John Abd-El-Malek]

Per our discussions, I've updated the WebKit code so that the embedder doesn't have to check for a PluginDocument.  You're right, it makes the API simpler.  The one area I've left as is is the find-in-page.  Since the (non-plugin) logic is inside chromium code, I couldn't move just the plugin case to the webkit side.

[John Abd-El-Malek]

Created attachment 60797 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 60797 [details]
Patch

WebKit/chromium/src/WebFrameImpl.h:203
 +      static WebPluginContainerImpl* pluginContainerFromFrame(WebCore::Frame* frame);
nit: leave off the parameter name

WebKit/chromium/public/WebPlugin.h:108
 +      // doesn't block the thread in case of a large document.  The results, along with the
nit: "is sent" -> "should be sent"?

otherwise, r=me

[John Abd-El-Malek]

Committed r62743: <http://trac.webkit.org/changeset/62743>