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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted 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 76298: webkitplugins.diff
https://bugs.webkit.org/attachment.cgi?id=76298&action=review

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