Bug 50827 – [GTK] Add APIs for plugin management

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Xan Lopez]

In order for clients to write something like about:plugins we need to expose APIs to get the list of active plugins in the session and some basic information about them. First shot at this.

[Xan Lopez]

Created attachment 76217 [details]
webkitplugins.diff

[Martin Robinson]

Comment on attachment 76217 [details]
webkitplugins.diff

View in context: https://bugs.webkit.org/attachment.cgi?id=76217&action=review

Great stuff. Lots of nitpicks below and a couple suggestions.

> WebKit/gtk/tests/testwebplugindatabase.c:21
> +#include <unistd.h>

unistd.h should go after the GLib/GTK+ stuff.

> WebKit/gtk/tests/testwebplugindatabase.c:27
> +#if GLIB_CHECK_VERSION(2, 16, 0) && GTK_CHECK_VERSION(2, 14, 0)

Do we still need this GLib version check?

> WebKit/gtk/tests/testwebplugindatabase.c:37
> +    WebKitWebPluginDatabasePluginList l;
> +    GSList* p;

These should probably be called pluginList and current. If possible in this language, they should be defined where they are first used, otherwise I'd say intialize them to 0.

> WebKit/gtk/webkit/webkitwebplugin.cpp:30
> +static void mimeTypeFree(WebKitWebPluginMIMEType* mimeType)

This should be freeMimeType so that the verb is first, since it's not API.

> WebKit/gtk/webkit/webkitwebplugin.cpp:48
> +static void webkit_web_plugin_finalize(GObject *object)

Asterisk with the typename.

> WebKit/gtk/webkit/webkitwebplugin.cpp:55
> +    WEBKIT_WEB_PLUGIN(object)->priv->~WebKitWebPluginPrivate();

This can just be delete priv. See below.

> WebKit/gtk/webkit/webkitwebplugin.cpp:60
> +static void webkit_web_plugin_class_init(WebKitWebPluginClass *klass)

Asterisk with the typename.

> WebKit/gtk/webkit/webkitwebplugin.cpp:64
> +    GObjectClass* gobject_class = (GObjectClass*)klass;

gobjectClass and a C++ style cast?

> WebKit/gtk/webkit/webkitwebplugin.cpp:68
> +    g_type_class_add_private(klass, sizeof(WebKitWebPluginPrivate));

See below as well

> WebKit/gtk/webkit/webkitwebplugin.cpp:71
> +static void webkit_web_plugin_init(WebKitWebPlugin *plugin)

Asterisk with the typename.

> WebKit/gtk/webkit/webkitwebplugin.cpp:76
> +    new (priv) WebKitWebPluginPrivate();

I think Company suggested that instead of using WEBKIT_WEB_PLUGIN_GET_PRIVATE and placement new, we can just just use new to allocate the private section here.

> WebKit/gtk/webkit/webkitwebplugin.cpp:85
> +    WebKitWebPluginPrivate* priv = plugin->priv;
> +
> +    priv->corePlugin = package;

Instead of caching something you only use once, I think it makes more sense to do plugin->priv->corePlugin = package.

> WebKit/gtk/webkit/webkitwebplugin.cpp:148
> +    if (!priv->mimeTypes) {

This should be an early return.

> WebKit/gtk/webkit/webkitwebplugin.h:42
> +
> +typedef struct _WebKitWebPluginMIMEType {
> +    char* name;
> +    char* description;
> +    char** extensions;
> +} WebKitWebPluginMIMEType;
> +

There should probably be some documentation for this structure. In particular to say that extensions is a GLib string list.

> WebKit/gtk/webkit/webkitwebplugin.h:43
> +typedef GSList * WebKitWebPluginMIMETypeList;

I think it makes more sense to just use GSList in the API. I think it makes it clearer how to use the API.

> WebKit/gtk/webkit/webkitwebplugindatabase.cpp:32
> +static void webkit_web_plugin_database_dispose(GObject *object)

Asterisk again.

> WebKit/gtk/webkit/webkitwebplugindatabase.cpp:43
> +    GObjectClass* gobject_class = (GObjectClass*)klass;

gobjectClass and a C++ style cast?

> WebKit/gtk/webkit/webkitwebplugindatabase.cpp:47
> +    g_type_class_add_private(klass, sizeof(WebKitWebPluginDatabasePrivate));

If you use the method I mentioned above, you'll need to remove this.

> WebKit/gtk/webkit/webkitwebplugindatabase.cpp:50
> +static void webkit_web_plugin_database_init(WebKitWebPluginDatabase *database)

Asterisk. :)

> WebKit/gtk/webkit/webkitwebplugindatabase.cpp:55
> +    new (priv) WebKitWebPluginDatabasePrivate();

Same suggestion here.

> WebKit/gtk/webkit/webkitwebplugindatabase.cpp:115
> +WebKitWebPlugin* webkit_web_plugin_database_get_plugin_for_mimetype(WebKitWebPluginDatabase* database, const char *mimeType)

Another traveling asterisk.

> WebKit/gtk/webkit/webkitwebplugindatabase.cpp:142
> +    return WEBKIT_WEB_PLUGIN_DATABASE(g_object_new(WEBKIT_TYPE_WEB_PLUGIN_DATABASE, 0));

This doesn't produce a warning?

> WebKit/gtk/webkit/webkitwebview.cpp:5162
> +    static WebKitWebPluginDatabase* database;

This should probably be initialized to 0 explicitly.

[Xan Lopez]

Created attachment 76298 [details]
webkitplugins.diff

Round two.

[Xan Lopez]

(In reply to comment #2)

I've pretty much done everything you asked for except...


> > WebKit/gtk/tests/testwebplugindatabase.c:37
> > +    WebKitWebPluginDatabasePluginList l;
> > +    GSList* p;
> 
> These should probably be called pluginList and current. If possible in this language, they should be defined where they are first used, otherwise I'd say intialize them to 0.

I left 'p' as it is, as IMHO using that letter for list iterators is more or less as common as using 'i' for integer counters. Also no initialization to 0, since both variables are set to an initial value right away.

I've also used a RefPtr for the PluginPackage inside WebKitWebPlugin, as discussed yesterday in person.

[Martin Robinson]

Comment on attachment 76298 [details]
webkitplugins.diff

View in context: https://bugs.webkit.org/attachment.cgi?id=76298&action=review

> WebKit/gtk/tests/testwebplugindatabase.c:37
> +    WebKitWebView* view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> +    WebKitWebPluginDatabase* database;
> +    GSList* pluginList, *p;
> +    gboolean found = FALSE;
> +

The style guide says nothing about this (yet, MUHAHA), so it's not worth arguing. Other reviewers have made me change undefined variables in the past.

> WebKit/gtk/tests/testwebplugindatabase.c:46
> +        if (g_strcmp0(webkit_web_plugin_get_name(plugin), "WebKit Test PlugIn") == 0 &&
> +            g_strcmp0(webkit_web_plugin_get_description(plugin), "Simple Netscape plug-in that handles test content for WebKit") == 0)

Please change this to something like below before landing:
         if (!g_strcmp0(webkit_web_plugin_get_name(plugin), "WebKit Test PlugIn") == 0 &&
             !g_strcmp0(webkit_web_plugin_get_description(plugin), "Simple Netscape plug-in that handles test content for WebKit") == 0)

> WebKit/gtk/tests/testwebplugindatabase.c:68
> +    g_critical("You will need at least glib-2.16.0 and gtk-2.14.0 to run the unit tests. Doing nothing now.");

Better update this message.

> WebKit/gtk/webkit/webkitwebplugin.cpp:160
> +        unsigned i;
> +        for (i = 0; i < extensions.size(); i++)
> +            mimeType->extensions[i] = g_strdup(extensions[i].utf8().data());

This can be for (unsigned i = 0; i < extensions.size(); i++) if you're feeling saucy.

> WebKit/gtk/webkit/webkitwebplugin.h:41
> + * @extensions: the extensions associated with this MIME type.

Might just want to say that this array is null terminated.

> WebKit/gtk/webkit/webkitwebplugindatabaseprivate.h:37
> +WebKitWebPluginDatabase* webkit_web_plugin_database_new(void);

Probably want to drop the void here.

[Xan Lopez]

Comment on attachment 76298 [details]
webkitplugins.diff

Committed in r73858.

[Xan Lopez]

Closing.