[Webkit-unassigned] [Bug 50827] [GTK] Add APIs for plugin management

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50827


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #76217|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2010-12-10 11:07:51 PST ---
(From update of attachment 76217)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.