[webkit-reviews] review denied: [Bug 80691] [Qt] X11 plugins need to be reworked for Qt5+WK1 : [Attachment 133229] Patch

[bugzilla-daemon at webkit.org]

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

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

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


Looks good in general, but I have a couple of nitpicks. I have the feeling that
there's a bit more code that can be shared and that there may be ways of
getting access to the things like window handles.

> Source/WebCore/plugins/qt/PluginPackageQt.cpp:143
> +    const QLatin1String pluginBlackList[] = {
> +	   QStringLiteral("skypebuttons")

Type mismatch :)

> Source/WebCore/plugins/qt/PluginPackageQt.cpp:150
> +    QString baseName = QFileInfo(static_cast<QString>(m_path)).baseName();
> +    for (unsigned i = 0; i < sizeof(pluginBlackList) /
sizeof(QLatin1String); ++i) {
> +	   if (baseName == pluginBlackList[i])
> +	       return false;
> +    }

Would it perhaps make sense to use a pattern here similar to PluginPackageWin
with a dedicated isPluginBlacklisted() method? (just for clarity)

> Source/WebCore/plugins/qt/PluginViewQt.cpp:122
> +#if HAVE(QT5)
> +    static Display* dedicatedDisplay = 0;
> +    if (!dedicatedDisplay)
> +	   dedicatedDisplay = XOpenDisplay(0);
> +    ASSERT(dedicatedDisplay);
> +    return dedicatedDisplay;
> +#else

Any particular reason for a dedicated display connection? Perhaps the xcb
implementation of QPlatformNativeInterface::nativeResourceForWindow could
return the default display if no window pointer is passed.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:133
> +#if HAVE(QT5)
> +    return XDefaultScreen(x11Display());
> +#else
> +    return QX11Info::appScreen();
> +#endif

Why not always use XDefaultScreen?

The Qt4 implementation appears to cache the return value of XDefaultScreen. I
think it would be good to check if recent xcb/xlib always does a round-trip or
also caches, and if it does cache then I don't see any harm in simply using
XDefaultScreen with all Qt versions (or we cache ourselves).

> Source/WebCore/plugins/qt/PluginViewQt.cpp:142
> +#if HAVE(QT5)
> +    return XDefaultRootWindow(x11Display());
> +#else
> +    return QX11Info::appRootWindow();
> +#endif

Ditto :)

> Source/WebCore/plugins/qt/PluginViewQt.cpp:151
> +#if HAVE(QT5)
> +    return XDefaultDepth(x11Display(), x11Screen());
> +#else
> +    return static_cast<QWidget*>(platformPluginWidget())->x11Info().depth();

> +#endif

Ditto ;)

> Source/WebCore/plugins/qt/PluginViewQt.cpp:160
> +#if HAVE(QT5)
> +    XSync(x11Display(), false);
> +#else
> +    QApplication::syncX();
> +#endif

Same here, perhaps we can avoid the #ifdefs altogether...

> Source/WebCore/plugins/qt/PluginViewQt.cpp:250
> +#if !HAVE(QT5)
>      if (platformPluginWidget()) {
>	   if (focused)
>	      
static_cast<QWidget*>(platformPluginWidget())->setFocus(Qt::OtherFocusReason);
> -    } else {
> +    } else
> +#endif

Hm, I suppose you commented this out because we cannot use QWidget methods
here? I recall running into a similar issue with widget visibility when
removing the widgets dependency from WebCore and I just added a hook in
QWebPageClient (setWidgetVisible). The same could be done for focus, to avoid
regressing here.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:417
> +#if HAVE(QT5)
> +    xEvent->xany.window = 0;
> +#else

Isn't there a way to still get the window handle for ownerWidget?

> Source/WebCore/plugins/qt/PluginViewQt.cpp:954
> +#if HAVE(QT5)
> +    if (m_isWindowed)
> +	   return false;
> +#else

I think the fact that we skip this branch for Qt 5 deserves a short explaining
comment.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:1024
> +	       bool found = getVisualAndColormap(depth, m_visual, m_colormap,
false);

Please use /* forceARGB32 = */ false instead of just false to increase the
readability of the code.