[webkit-reviews] review denied: [Bug 70023] [Qt] X11 plugins need to be reworked for Qt5 : [Attachment 113932] wk2 plugins

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 70023: [Qt] X11 plugins need to be reworked for Qt5
https://bugs.webkit.org/show_bug.cgi?id=70023

Attachment 113932: wk2 plugins
https://bugs.webkit.org/attachment.cgi?id=113932&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113932&action=review


This is a good start, but it needs some work. I've added comments. Please also
do fix the coding style in the function that you took from Qt.

> Source/WebKit2/ChangeLog:45
> +	   affect if PLUGIN_ARCHITECTURE_UNSOPPORTED is not defined.

Typo.

> Source/WebKit2/WebProcess/Plugins/Netscape/qt/QtPluginHelper.cpp:27
> +/*
> + * Copyright (C) 2011 Nokia Corporation and/or its subsidiary(-ies).
> + * All rights reserved.
> + *
> + * GNU Lesser General Public License Usage
> + * This file may be used under the terms of the GNU Lesser General Public
> + * License version 2.1 as published by the Free Software Foundation and
> + * appearing in the file LICENSE.LGPL included in the packaging of this
> + * file. Please review the following information to ensure the GNU Lesser
> + * General Public License version 2.1 requirements will be met:
> + * http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
> + *
> + * In addition, as a special exception, Nokia gives you certain additional
> + * rights. These rights are described in the Nokia Qt LGPL Exception
> + * version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
> + *
> + * GNU General Public License Usage
> + * Alternatively, this file may be used under the terms of the GNU General
> + * Public License version 3.0 as published by the Free Software Foundation
> + * and appearing in the file LICENSE.GPL included in the packaging of this
> + * file. Please review the following information to ensure the GNU General
> + * Public License version 3.0 requirements will be met:
> + * http://www.gnu.org/copyleft/gpl.html.
> + *
> + * Note: The function qimageFromXImage has been taken from the Qt Toolkit.
> + *
> + */

I think the part of the license text that grants the special exception is not
transferrable and should be removed. Similarly we do not intend
to take GPL licensed code into WebKit. As a result this file is licensed solely
under the LGPL.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:102
>  static inline Display* x11Display()
>  {
>  #if PLATFORM(QT)
> -    return QX11Info::display();
> +    static bool opened = false;
> +    if (!opened) {
> +	   NetscapePlugin::s_fakeDisplayForPlugins = XOpenDisplay(0);
> +	   opened = true;
> +    }
> +    return NetscapePlugin::s_fakeDisplayForPlugins;
>  #elif PLATFORM(GTK)
>      return GDK_DISPLAY_XDISPLAY(gdk_display_get_default());
>  #else

I'd prefer this code to turn into a proper public class method of
NetscapePlugin. The "fakeDisplayForPlugins" variable can become "static
Display* dedicatedPluginDisplay;", local to the function.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:168
> +    if (!display)
> +	   return false;

This should be an ASSERT, not a return false.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:174
> +#if PLATFORM(QT)
> +    if (depth != 16 && depth != 24 && depth != 32)
> +	   return false;
> +#endif

I don't think that this should be a run-time check with "return false". I'd
rather like to see this as an ASSERT. It is fatal.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:187
> -    ASSERT(visualInfo);
> +    if (!visualInfo)
> +	   return false;

Why did you replace the ASSERT with a return? I think an ASSERT is the right
thing to use here, the lack of a visual
should be fatal to us.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:284
> +    XImage* xImage = XGetImage(x11Display(), m_drawable, exposedRect.x(),
exposedRect.y(),
> +				  exposedRect.width(), exposedRect.height(),
ULONG_MAX, ZPixmap);

I believe this will leak for each paint. You have to destroy the XImage with
XDestroyImage.

> Source/WebKit2/config.h:160
> -#elif PLATFORM(GTK) && (OS(UNIX) && !OS(MAC_OS_X))
> +#elif (PLATFORM(QT) || PLATFORM(GTK)) && (OS(UNIX) && !OS(MAC_OS_X))

This doesn't look right when considering that Qt can be compiled for say Linux
without xcb support. In that case
we should _not_ set PLUGIN_ARCHITECTURE_X11.

I think perhaps this whole block of #ifdefs should be changed to allow us to
define the plugin architecture
from within the .pro files instead of at compile time.

So you could for example surround the whole thing by #ifndef
PLUGIN_ARCHITECTURE_SET and set it accordingly, along
with PLUGIN_ARCHITECTURE_X11 if plugins_X11 is set.

> Tools/qmake/mkspecs/features/features.prf:100
> -    unix:!haveQt(5)|win32-*:!embedded:!wince*: {
> -	   DEFINES += ENABLE_NETSCAPE_PLUGIN_API=1
> +    !haveQt(5): {
> +	   unix|win32-*:!embedded:!wince*: {
> +	       DEFINES += ENABLE_NETSCAPE_PLUGIN_API=1
> +	   } else {
> +	       DEFINES += ENABLE_NETSCAPE_PLUGIN_API=0
> +	   }
>      } else {
>	   DEFINES += ENABLE_NETSCAPE_PLUGIN_API=0
>      }

Hang on, don't we want to re-enable NPAPI on Qt 5, too?

> Tools/qmake/mkspecs/features/features.prf:104
> +!no_webkit2:!contains(DEFINES, PLUGIN_ARCHITECTURE_UNSOPPORTED): {

Typo: UNSOPPORTED -> UNSUPPORTED

> Tools/qmake/mkspecs/features/features.prf:108
> +	   DEFINES += PLUGIN_ARCHITECTURE_UNSOPPORTED

Typo

> LayoutTests/platform/qt-wk1/Skipped:5
> +
> +# [Qt] Plugins need to be reworked for Qt5
> +# https://bugs.webkit.org/show_bug.cgi?id=70023
> +plugins

This doesn't look right. Don't we want to continue to test plugins with wk1 and
Qt 4.x?

AFAICS in qt.py, qt-wk1/Skipped is used for builds that are _not_ webkit2,
regardless of the Qt version.

We should only skip these plugin when running WebKit1 tests with a build of
WebKit against Qt 5.