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

[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 114074: wk2 plugins v2 fixed
https://bugs.webkit.org/attachment.cgi?id=114074&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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 :)