Bug 27210

Summary: [Qt] Add an API to get a list of installed plugins
Product: WebKit Reporter: Jakub Wieczorek <jwieczorek>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: laszlo.gombos, manyoso
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch manyoso: review-

Description Jakub Wieczorek 2009-07-13 03:37:20 PDT
Created attachment 32656 [details]
patch

The attached patch adds a new function: QWebPluginFactory::installedPlugins() that returns a list of plugins currently installed and recognized by WebKit, with an autotest.
Comment 1 Adam Treat 2009-07-14 07:15:45 PDT
Comment on attachment 32656 [details]
patch

We talked this over in #qtwebkit and believe this is a great addition to the API.  However, there might be confusion with two different functions that appear to do the same thing: plugins() and installedPlugins().  With this in mind, how about a method such as 'QList<QWebPluginFactory::Plugin> QWebPage::plugins(enum);' where the enum would differentiate between system plugins that are picked up by scanning netscape plugin directories and those plugins that are installed via QWebPluginFactory.

With this change, perhaps QWebPluginFactory::Plugin should have it's own class such as QWebPlugin.  I'm not sure.

Eventually, it'd be nice to add API to set/get the preferred plugin for a particular mimetype and also to query/set the directories searched for system plugins.  Some ideas for how to do that are here:

http://code.staikos.net/cgi-bin/gitweb.cgi?p=webkit;a=shortlog;h=adam/netscape-plugin-manager

Cheers,
Adam
Comment 2 Jakub Wieczorek 2009-07-14 09:03:11 PDT
> We talked this over in #qtwebkit and believe this is a great addition to the
> API.  However, there might be confusion with two different functions that
> appear to do the same thing: plugins() and installedPlugins().  With this in
> mind, how about a method such as 'QList<QWebPluginFactory::Plugin>
> QWebPage::plugins(enum);' where the enum would differentiate between system
> plugins that are picked up by scanning netscape plugin directories and those
> plugins that are installed via QWebPluginFactory.

I intentionally made my function a static one. This way one doesn't have to reimplement and instantiate a custom plugin factory just to get a list of installed plugins.

Also, once we get more functions that concern netscape plugins such as setPreferredPlugin or whatever else, they should also be static IMO. Mostly because one can have different plugin factory per each page and thus it should be emphasised in the API that these functions would affect all pages (because they do), not only those that hold a particular plugin factory. And if those functions were static, it's another reason that installedPlugins should also be static, for consistency.

I'm also not sure if the API suggested by you would actually be less confusing. We obviously would have to keep the virtual plugins() function (the one without arguments), which is supposed to be reimplemented in subclasses. This way we would end up with two public functions:

virtual QList<Plugin> plugins() const;
QList<Plugin> plugins(Type type) const; - which would call plugins() when type == FactoryPlugins.

Just not sure if it makes more sense. It would do if the virtual function was private but we can't change the visibility obviously.
 
> With this change, perhaps QWebPluginFactory::Plugin should have it's own class
> such as QWebPlugin.  I'm not sure.

This isn't essential for this particular patch, nor for any further. This would also break BC, even if we typedefed it.

> Eventually, it'd be nice to add API to set/get the preferred plugin for a
> particular mimetype and also to query/set the directories searched for system
> plugins.  Some ideas for how to do that are here:
> 
> http://code.staikos.net/cgi-bin/gitweb.cgi?p=webkit;a=shortlog;h=adam/netscape-plugin-manager

Sure, I'm going to work on this API, I looked at your branch and am most likely going to grab some ideas, thanks!

> 
> Cheers,
> Adam

Thanks for the comments.