[webkit-reviews] review denied: [Bug 50827] [GTK] Add APIs for plugin management : [Attachment 76217] webkitplugins.diff

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 50827: [GTK] Add APIs for plugin management
https://bugs.webkit.org/show_bug.cgi?id=50827

Attachment 76217: webkitplugins.diff
https://bugs.webkit.org/attachment.cgi?id=76217&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.