Bug 70023 – [Qt] X11 plugins need to be reworked for Qt5

• Component: Plug-ins
• Status: RESOLVED FIXED

[Balazs Kelemen]

The current plugin code - both WK1 and WK2 - does not build with Qt5 because we don't have QPixmap::fromX11Pixmap anymore. On the bots it is hided by #69931. To fix this we should rethink how plugins should work with QPA. Until that we should explicitly forbid plugins with Qt5 (and not depend on the existence of the qmake bug).

[Balazs Kelemen]

Created attachment 110841 [details]
Patch

[Simon Hausmann]

Comment on attachment 110841 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110841&action=review

> Source/WebCore/features.pri:91
> +    unix:!qt5|win32-*:!embedded:!wince*: {

Hrm, what is the precedence here?

It is one big complex condition that is hard to understand :)

It looks like the ":" serves as logical and operator, which means after your change
plugins will only be enabled if "unix" _AND_ (!qt5 _or win32). How can the configuration
be unix _AND_ win32?

Previously it was win32 _OR_ unix.

[Balazs Kelemen]

(In reply to comment #2)
> (From update of attachment 110841 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110841&action=review
> 
> > Source/WebCore/features.pri:91
> > +    unix:!qt5|win32-*:!embedded:!wince*: {
> 
> Hrm, what is the precedence here?
> 
> It is one big complex condition that is hard to understand :)
> 
> It looks like the ":" serves as logical and operator, which means after your change
> plugins will only be enabled if "unix" _AND_ (!qt5 _or win32). How can the configuration
> be unix _AND_ win32?
> 
> Previously it was win32 _OR_ unix.

I believe : is stronger than | as in normal logic :), so it is (unix and !qt5) or (win32 and ...), but I can add parentheses to make it clear.

[Balazs Kelemen]

> I believe : is stronger than | as in normal logic :), so it is (unix and !qt5) or (win32 and ...), but I can add parentheses to make it clear.

No I can't, qmake doesn't understand it :( , however I tried with an example and the condition is true on Linux with qt4 qmake and false with qt5 so it seems like my theory about precedence is correct.

[Balazs Kelemen]

Created attachment 112867 [details]
Patch

[Balazs Kelemen]

Skip plugin tests through the skipped list for Qt5, and not remove the unexplained failures from the list (maybe those will fail in the future as well). Otherwise the same patch.

[Simon Hausmann]

Comment on attachment 112867 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112867&action=review

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

Hm, won't this end up disabling plugins for the Qt 4.x/X11 based build in trunk?

[Simon Hausmann]

I think we need at least two steps in this, to at least get windowless plugins on X11:

1) Figure out whether we're on X11 or not. Either at run-time or build time.
2) Replace the QPixmap code in NetscapePluginX11.cpp with plain X11 code to retrieve the image and then wrap it in a QImage.


In the long run I wonder if we can add X11 pixmap support to the texture mapper (i.e. layers consisting of an X11 pixmap) on the WebProcess and UIProcess side (using TextureFromPixmap for rendering). That should make the rendering of windowless plugins on X11 really efficient.

[Balazs Kelemen]

(In reply to comment #7)
> (From update of attachment 112867 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112867&action=review
> 
> > Source/WebKit2/config.h:161
> > -#elif (PLATFORM(QT) || (PLATFORM(GTK))) && (OS(UNIX) && !OS(MAC_OS_X))
> > +#elif PLATFORM(GTK) && (OS(UNIX) && !OS(MAC_OS_X))
> >  #define PLUGIN_ARCHITECTURE_X11 1
> 
> Hm, won't this end up disabling plugins for the Qt 4.x/X11 based build in trunk?

Qt 4.x + WebKit2 is not supported. http://trac.webkit.org/browser/trunk/Source/WebKit.pro?#L12 :)

[Balazs Kelemen]

(In reply to comment #8)
> I think we need at least two steps in this, to at least get windowless plugins on X11:
> 
> 1) Figure out whether we're on X11 or not. Either at run-time or build time.
> 2) Replace the QPixmap code in NetscapePluginX11.cpp with plain X11 code to retrieve the image and then wrap it in a QImage.

Sounds like a plan. I stalled with this because I don't know how to get the X connection with Qt5 (the Display* which is needed to call xlib functions). QX11Info is not functional anymore. It's there but it does not have an implementation -> link error :(

[Simon Hausmann]

(In reply to comment #10)
> (In reply to comment #8)
> > I think we need at least two steps in this, to at least get windowless plugins on X11:
> > 
> > 1) Figure out whether we're on X11 or not. Either at run-time or build time.
> > 2) Replace the QPixmap code in NetscapePluginX11.cpp with plain X11 code to retrieve the image and then wrap it in a QImage.
> 
> Sounds like a plan. I stalled with this because I don't know how to get the X connection with Qt5 (the Display* which is needed to call xlib functions). QX11Info is not functional anymore. It's there but it does not have an implementation -> link error :(

You'll probably have to open a new display connection yourself. Qt is not using xlib but xcb. Flash most probably expects that the application uses Xlib, so that's probably what we'll have to do in WebKit.

[Simon Hausmann]

Actually a solution for run-time detection is to use the QPlatformNativeInterface, which for X11 for example can retrieve the display (when xcb is using xlib).

We still need a way to reliably detect the presence of xlib, so that we can at least build the x11 code and later detect and enable it at run-time.

Right now what would work is

contains(QT_CONFIG, xcb-xlib)

Then we know at least that Qt is built at least with the xcb plugin with xlib support enabled. There may be another plugin enabled at run-time, but at least there's a remote possibility that the xcb-xlib one will show up at some point :)

[Balazs Kelemen]

Comment on attachment 112867 [details]
Patch

Landed in http://trac.webkit.org/changeset/99163

[Balazs Kelemen]

Created attachment 113932 [details]
wk2 plugins

Rework our basic WK2 plugin support

[WebKit Review Bot]

Attachment 113932 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1

Source/WebKit2/WebProcess/Plugins/Netscape/qt/QtPluginHelper.cpp:50:  Missing spaces around =  [whitespace/operators] [4]
Source/WebKit2/WebProcess/Plugins/Netscape/qt/QtPluginHelper.cpp:52:  Declaration has space between type name and * in ushort *p  [whitespace/declaration] [3]
Source/WebKit2/WebProcess/Plugins/Netscape/qt/QtPluginHelper.cpp:53:  Declaration has space between type name and * in ushort *end  [whitespace/declaration] [3]
Source/WebKit2/WebProcess/Plugins/Netscape/qt/QtPluginHelper.cpp:59:  Declaration has space between type name and * in uint *p  [whitespace/declaration] [3]
Source/WebKit2/WebProcess/Plugins/Netscape/qt/QtPluginHelper.cpp:60:  Declaration has space between type name and * in uint *end  [whitespace/declaration] [3]
Source/WebKit2/WebProcess/Plugins/Netscape/qt/QtPluginHelper.cpp:72:  Declaration has space between type name and * in QRgb *p  [whitespace/declaration] [3]
Total errors found: 6 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

(In reply to comment #14)
> Created an attachment (id=113932) [details]
> wk2 plugins
> 
> Rework our basic WK2 plugin support

It's still don't do anything for plugin process or windowed plugins. Just rework what we had before.
Maybe we also need a runtime check to know what platform plugin we have (xcb, wayland, whatever), I'm not sure. If that's the case then we need to wait for that API to appear in Qt5.
Tested with flash. There was no observable problem with the frame rate.

[Balazs Kelemen]

The style issues are associated with the Qt code. I took it from Qt so I wouldn't like to change it.

[Simon Hausmann]

Comment on attachment 113932 [details]
wk2 plugins

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.

[Balazs Kelemen]

> > 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.

These conditions are fatal but only for running the plugin. If we return false here then the plugin will be abandoned and we won't be in trouble later. I changed the assert to if because we cannot really control these conditions (the display, the depth, and the visual). Do you still think these should be asserts?


> > 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.

Currently it is surrounded with PLUGIN_ARCHITECTURE_UNSUPPORTED and I used that to prevent turning it on for non xcb-xlib platforms. However you are right in that it would be clearer to set this up solely in the .pro files.

> > 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?

Ok, it's not really clear. So, ENABLE_NETSCAPE_PLUGIN_API controls it for WK1, PLUGIN_ARCHITECTURE_??? controls it for WK2. Maybe we should introduce a common config condition for these two, do you think so? Otherwise, do you mean we should fix plugins+Qt5 for WK1 as well? Is the old API supposed to be released with Qt5? Anyhow, it need additional work to fix it for WK1. The WK1 path also has a wider support (windowless/windowed, transparency) so I guess it would not be trivial. I would go with only WK2 in this first step.
 
> > 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.

Sure, it was a silly.

[Simon Hausmann]

(In reply to comment #19)
> > > 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.
> 
> These conditions are fatal but only for running the plugin. If we return false here then the plugin will be abandoned and we won't be in trouble later. I changed the assert to if because we cannot really control these conditions (the display, the depth, and the visual). Do you still think these should be asserts?

Well, but if the conditions fail, then it's not because of anything specific to the plugin, it means something is seriously busted on the host side,
preventing us from loading any plugin. That's why I think this justifies an ASSERT. 
 
> > > 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?
> 
> Ok, it's not really clear. So, ENABLE_NETSCAPE_PLUGIN_API controls it for WK1, PLUGIN_ARCHITECTURE_??? controls it for WK2. Maybe we should introduce a common config condition for these two, do you think so? Otherwise, do you mean we should fix plugins+Qt5 for WK1 as well? Is the old API supposed to be released with Qt5? Anyhow, it need additional work to fix it for WK1. The WK1 path also has a wider support (windowless/windowed, transparency) so I guess it would not be trivial. I would go with only WK2 in 
this first step.

Ok, I see. Yes, unfortunately we need to support WK1 with Qt 5, and that includes plugin support :(. That can however easily be a separate "project".

Oh, that reminds me: Then we can continue to use the plugin directory code from WebCore/plugins, no?

[Balazs Kelemen]

> Oh, that reminds me: Then we can continue to use the plugin directory code from WebCore/plugins, no?

It is not working. With ENABLE_NETSCAPE_PLUGIN_API=0 - which is necessary as WK1 path does not build with Qt5 - we don't build it. I think copying a few simple line is worth to remove the dependency :)

Another issue: I won't be able to enable tests because there is no way to enable them for Qt5-WK2 and Qt4-WK1 but not for Qt5-WK1.

[Balazs Kelemen]

Created attachment 114061 [details]
wk2 plugins v2

[Early Warning System Bot]

Comment on attachment 114061 [details]
wk2 plugins v2

Attachment 114061 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10369114

[Balazs Kelemen]

Created attachment 114074 [details]
wk2 plugins v2 fixed

[Simon Hausmann]

Comment on attachment 114074 [details]
wk2 plugins v2 fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=114074&action=review

Good! This is much better. We're close now. I think one more iteration is needed. Comments below.

> Source/WebCore/ChangeLog:21
> +        * plugins/qt/QtX11ImageConversion.cpp: Added.
> +        (WebCore::qimageFromXImage):
> +        * plugins/qt/QtX11ImageConversion.h: Added.

On second thought, isn't it a bit strange to add a file into WebCore that is used only by WebKit2?

What about for example WebKit2/Shared/Plugins/qt?

> Source/WebCore/plugins/qt/QtX11ImageConversion.cpp:12
> +/*
> + * 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.
> + *

Ok, after taking another look at this, please use the following license text that we use for code contributions from Nokia:

----

/*
 * Copyright (C) 2011 Nokia Corporation and/or its subsidiary(-ies)
 *
 * This library is free software; you can redistribute it and/or
 * modify it under the terms of the GNU Library General Public
 * License as published by the Free Software Foundation; either
 * version 2 of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 * Library General Public License for more details.
 *
 * You should have received a copy of the GNU Library General Public License
 * along with this program; see the file COPYING.LIB.  If not, write to
 * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
 * Boston, MA 02110-1301, USA.
 *
 */
----

> Source/WebCore/plugins/qt/QtX11ImageConversion.cpp:38
> +                ushort * p = reinterpret_cast<ushort*>(image.scanLine(i));

The coding style check missed out the placement of the * :)

> Source/WebCore/plugins/qt/QtX11ImageConversion.h:13
> +/*
> + * 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.
> + *
> + */

(Same here btw regarding the license text)

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:269
> -    if (m_pluginDisplay != x11Display())
> +    if (m_pluginDisplay != pluginDisplay())

This line makes it clear that we are having a naming problem: If the plugin display is not the plugin display? Huh?
The file has

    NetscapePlugin::pluginDisplay()
    getPluginDisplay()
and
    m_pluginDisplay

I suggest to rename the class method to x11HostDisplay(), so that the code becomes

    if (m_pluginDisplay != x11HostDisplay())

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:280
> +    QImage image = qimageFromXImage(xImage);
> +
>      QPainter* painter = context->platformContext();
> -    painter->drawPixmap(QPoint(exposedRect.x(), exposedRect.y()), qtDrawable, exposedRect);
> +    painter->drawImage(QPoint(exposedRect.x(), exposedRect.y()), image, exposedRect);
> +
> +    image = QImage();

Instead of clearing the QImage manually, I suggest the use of a temporary and write:

painter->drawImage(QPoint(...), qImageFromXImage(xImage), exposedRect())

> Tools/qmake/mkspecs/features/features.prf:105
> +    contains(QT_CONFIG, xcb-xlib) {

Note: In the future we can replace this "check" about the availability of X11 by using the new module config test feature of the Qt 5 build system. Not necessary for this patch though :)

[Balazs Kelemen]

(In reply to comment #25)
> (From update of attachment 114074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114074&action=review
> 
> Good! This is much better. We're close now. I think one more iteration is needed. Comments below.
> 
> > Source/WebCore/ChangeLog:21
> > +        * plugins/qt/QtX11ImageConversion.cpp: Added.
> > +        (WebCore::qimageFromXImage):
> > +        * plugins/qt/QtX11ImageConversion.h: Added.
> 
> On second thought, isn't it a bit strange to add a file into WebCore that is used only by WebKit2?

I added it here to be able to reuse for WK1 in the future - as it is suggested in the changelog ;) Do you think it's reasonable?

> 
> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:269
> > -    if (m_pluginDisplay != x11Display())
> > +    if (m_pluginDisplay != pluginDisplay())
> 
> This line makes it clear that we are having a naming problem: If the plugin display is not the plugin display? Huh?
> The file has
> 
>     NetscapePlugin::pluginDisplay()
>     getPluginDisplay()
> and
>     m_pluginDisplay
> 
> I suggest to rename the class method to x11HostDisplay(), so that the code becomes
> 
>     if (m_pluginDisplay != x11HostDisplay())

Actually I plan to remove the getPluginDisplay function and the m_pluginDisplay member. It is only used to determine whether we should call XSync. Actually the results of the whole thing is calling it for Qt and not for GTK so it's better to get rid of that. I planned to do this in another patch but then let's do it here. However x11HostDisplay is a more clear name, I will do the renaming.

> 
> > Tools/qmake/mkspecs/features/features.prf:105
> > +    contains(QT_CONFIG, xcb-xlib) {
> 
> Note: In the future we can replace this "check" about the availability of X11 by using the new module config test feature of the Qt 5 build system. Not necessary for this patch though :)

I'm going to take a look to this fancy feature. :)

[Balazs Kelemen]

> > > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:269
> > > -    if (m_pluginDisplay != x11Display())
> > > +    if (m_pluginDisplay != pluginDisplay())
> > 
> > This line makes it clear that we are having a naming problem: If the plugin display is not the plugin display? Huh?
> > The file has
> > 
> >     NetscapePlugin::pluginDisplay()
> >     getPluginDisplay()
> > and
> >     m_pluginDisplay
> > 
> > I suggest to rename the class method to x11HostDisplay(), so that the code becomes
> > 
> >     if (m_pluginDisplay != x11HostDisplay())
> 
> Actually I plan to remove the getPluginDisplay function and the m_pluginDisplay member. It is only used to determine whether we should call XSync. Actually the results of the whole thing is calling it for Qt and not for GTK so it's better to get rid of that. I planned to do this in another patch but then let's do it here. However x11HostDisplay is a more clear name, I will do the renaming.

Ok, I have just realized that this won't work because we need the display used by the plugin to sync with, i.e. XSysnc(m_pluginDisplay, false). So I'm going to do it as you suggested.

[Balazs Kelemen]

Created attachment 114494 [details]
wk2 plugins v3

[Balazs Kelemen]

(In reply to comment #28)
> Created an attachment (id=114494) [details]
> wk2 plugins v3

Incorporated review comments with the exception of
 - still put the new files to WebCore
 - postpone the build time x11 test to a next step

[Simon Hausmann]

Comment on attachment 114494 [details]
wk2 plugins v3

View in context: https://bugs.webkit.org/attachment.cgi?id=114494&action=review

Nice! r=me

> Source/WebCore/ChangeLog:25
> +        it for the WK1 plugin support in the future.

Alright, I had overlooked that point :)

[Balazs Kelemen]

Comment on attachment 114494 [details]
wk2 plugins v3

Clearing flags on attachment: 114494

Committed r99872: <http://trac.webkit.org/changeset/99872>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

Reopen because wk1+qt5 is still need to be reworked (as well as wk2 + pluginprocess but that's another story). Also the skiplist refers to this bug.