[webkit-reviews] review denied: [Bug 66650] [Qt] Flash in a QGraphicsWebView using OpenGL is slow : [Attachment 113167] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Christophe Oosterlynck
<christophe.oosterlynck at dzine.be>'s request for review:
Bug 66650: [Qt] Flash in a QGraphicsWebView using OpenGL is slow
https://bugs.webkit.org/show_bug.cgi?id=66650

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
I'm sorry, I'm giving this an r-. At the very least because the ChangeLog
doesn't make much sense
by saying that "Plugins are now drawn on a QPixmap instead of on a X11 Pixmap
when the OpenGL backend is used".

X11 windowless plugins can only be rendered onto X11 pixmaps (technically it's
a general purpose X11 drawable, but a pixmap
by all means). The following line is responsible for passing the information
_where_ the plugin should paint to:

    exposeEvent.drawable = qtDrawable.handle();

That means qtDrawable must be a QPixmap that is backed by an X11 pixmap.

This is the case with the current code and unless you're using the OpenGL
graphics system, a regular QPixmap instance allocated
with Qt compiled for X11 will _also_ be backed by an x11 pixmap. So the
statement "...draw onto QPixmap instead of on an X11 Pixmap" doesn't make sense
because QPixmap is _also_ backed by an X11 pixmap, returned by handle().

If however the OpenGL graphics system is used, then QPixmaps will not be backed
by X11 pixmaps and QPixmap::handle() will return 0. A 0
drawable is certainly not going to make the plugin paint.


So either I am missing something (in which case please elaborate in the
ChangeLog) or the patch will not have any effect and certainly break plugin
rendering when the OpenGL graphics system is used, because QPixmap::handle()
will return 0 unless the QPixmap was constructed using QPixmap::fromX11Pixmap.