Bug 62200

Created attachment 96224 [details] Patch Gustavo, I changed my mind about making moduleMixesGtkSymbols() public, because the implementation is not exactly the same for wk1 and wk2, so I added PluginPackage::module() instead.

Comment on attachment 96224 [details] Patch Removing flags, sorry, I made the patch from uncommitted changes.

Created attachment 96225 [details] Updated patch to apply to curent git master

Comment on attachment 96225 [details] Updated patch to apply to curent git master 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.

(In reply to comment #4) > (From update of attachment 96225 [details]) > 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? We need to be able to load the plugin even if it mixes gtk symbols in webkit2. To load the plugin and get the info the ui process uses PluginPackage. The problem of mixing symbols is in the view, that uses gtk. If we leave the check in PlugionPackage the ui process won't be able to load the plugin and the plugin process will never be launched. > > 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? Because the implementations are not exactly the same, in webkit2 we don't have access to the platform module, but we have a method in Module to get a symbol from the module. > > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:813 > > + gpointer symbol; > > It's probably better to use void* here. That's existing code, not mine, I'll change it anyway, gpointer is indeed void * > > 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. Ok.

(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 96225 [details] [details]) > > 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? > > We need to be able to load the plugin even if it mixes gtk symbols in webkit2. To load the plugin and get the info the ui process uses PluginPackage. The problem of mixing symbols is in the view, that uses gtk. If we leave the check in PlugionPackage the ui process won't be able to load the plugin and the plugin process will never be launched. Okay. Do you mind expanding the ChangeLog to mention that PluginPackage::load is used by both WebKit1 and WebKit2, so this necessitates moving the check?

Comment on attachment 96225 [details] Updated patch to apply to curent git master r=me with the aforementioned changes!

(In reply to comment #6) > Okay. Do you mind expanding the ChangeLog to mention that PluginPackage::load is used by both WebKit1 and WebKit2, so this necessitates moving the check? Sure, thanks for reviewing!

Committed r88244: <http://trac.webkit.org/changeset/88244>