[webkit-reviews] review denied: [Bug 58580] Make plugins compile during WebKit GTK Windows build : [Attachment 89663] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Fridrich Strba
<fridrich.strba at bluewin.ch>'s request for review:
Bug 58580: Make plugins compile during WebKit GTK Windows build
https://bugs.webkit.org/show_bug.cgi?id=58580

Attachment 89663: Patch
https://bugs.webkit.org/attachment.cgi?id=89663&action=review

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

Looks good, but I think it needs another iteration.

> Source/WebCore/plugins/PluginView.h:366
> +#if defined(XP_UNIX) || PLATFORM(GTK)
>	   bool m_needsXEmbed;

Is the m_needsXEmbed variable really necessary for Windows?

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:83
> +#include <windows.h>
>  #include "PluginMessageThrottlerWin.h"
>  #include <gdk/gdkwin32.h>

Please put these in alphabetical order if you can.

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:667
> -	       *static_cast<HGIOBJ*>(value) = GDK_WINDOW_HWND(gdkWindow);
> +	       *static_cast<HGDIOBJ*>(value) = GDK_WINDOW_HWND(gdkWindow);

Whoops. I think this was my fault.

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:798
>	   PluginView::setCurrentPluginView(this);
>	   JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);
>	   setCallingPlugin(true);
> +#if defined(XP_UNIX)
>	   m_plugin->pluginFuncs()->getvalue(m_instance, NPPVpluginNeedsXEmbed,
&m_needsXEmbed);
> +#endif

If you're going to avoid the plugin call on !XP_UNIX, it makes sense to avoid
all the preparation for the call as well.