Bug 50827 - [GTK] Add APIs for plugin management
: [GTK] Add APIs for plugin management
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-12-10 10:21 PST by
Modified: 2010-12-11 08:31 PST (History)


Attachments
webkitplugins.diff (30.84 KB, patch)
2010-12-10 10:30 PST, Xan Lopez
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
webkitplugins.diff (30.73 KB, patch)
2010-12-11 02:55 PST, Xan Lopez
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-12-10 10:21:41 PST
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.
------- Comment #1 From 2010-12-10 10:30:52 PST -------
Created an attachment (id=76217) [details]
webkitplugins.diff
------- Comment #2 From 2010-12-10 11:07:51 PST -------
(From update of attachment 76217 [details])
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.
------- Comment #3 From 2010-12-11 02:55:20 PST -------
Created an attachment (id=76298) [details]
webkitplugins.diff

Round two.
------- Comment #4 From 2010-12-11 02:57:14 PST -------
(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.
------- Comment #5 From 2010-12-11 07:46:30 PST -------
(From update of attachment 76298 [details])
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.
------- Comment #6 From 2010-12-11 08:31:38 PST -------
(From update of attachment 76298 [details])
Committed in r73858.
------- Comment #7 From 2010-12-11 08:31:48 PST -------
Closing.