[webkit-reviews] review denied: [Bug 62200] [GTK] Move moduleMixesGtkSymbols() from PluginPackage to PluginView : [Attachment 96225] Updated patch to apply to curent git master

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 62200: [GTK] Move moduleMixesGtkSymbols() from PluginPackage to PluginView
https://bugs.webkit.org/show_bug.cgi?id=62200

Attachment 96225: Updated patch to apply to curent git master
https://bugs.webkit.org/attachment.cgi?id=96225&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96225&action=review

The bits in WebKit2 look good, but it seems we don't really need to move any
WebKit1 code around for this change. Do you mind undoing that bit of the patch?


> Source/WebCore/plugins/gtk/PluginPackageGtk.cpp:-169
> -    if (moduleMixesGtkSymbols(m_module)) {
> -	   LOG(Plugins, "Module '%s' mixes GTK+ 2 and GTK+ 3 symbols, ignoring
plugin.\n", m_path.utf8().data());
> -	   g_module_close(m_module);
> -	   return false;
> -    }
> -

I think it still makes sense to do this check here instead of creating a dead
plugin. Why not just make the moduleMixesGtkSymbols a static method on
PluginView and call it here?

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:813
> +    gpointer symbol;

It's probably better to use void* here.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:138
> +    return module->functionPointer<gpointer>("gtk_application_get_type");

Again probably better to use void* here directly.