[webkit-reviews] review denied: [Bug 34681] [Qt] the above website does not render properly when in compositing mode : [Attachment 48302] GraphicsLayerQt: masks are now being handled outside the layer's paintEvent

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at gmail.com> has denied Noam Rosenthal
<noam.rosenthal at nokia.com>'s request for review:
Bug 34681: [Qt] the above website does not render properly when in compositing
mode
https://bugs.webkit.org/show_bug.cgi?id=34681

Attachment 48302: GraphicsLayerQt: masks are now being handled outside the
layer's paintEvent
https://bugs.webkit.org/attachment.cgi?id=48302&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at gmail.com>
> +class MaskEffectQt : public QGraphicsEffect {

If we still need to support Qt 4.5, better enclose this in #ifdef QT_VERSION
for 4.6 and beyond.
And yes, there should be an alternative code path for Qt 4.5 even if it means
the test site will not work.

> +	   QPixmap srcPixmap = sourcePixmap(Qt::LogicalCoordinates, &offset,
QGraphicsEffect::NoPad);
> +	   QPixmap pixmap(srcPixmap.size());
> +	   pixmap.fill(Qt::transparent);
> +
> +	   if (pixmap.isNull())
> +	       return;
> +
> +	   QPainter pixmapPainter(&pixmap);
> +	   pixmapPainter.setRenderHints(painter->renderHints());
> +	   pixmapPainter.setCompositionMode(QPainter::CompositionMode_Source);
> +	   pixmapPainter.drawPixmap(0, 0, srcPixmap);

That sounds inefficient. Why do you need to create 'pixmap', fill it
transparent, then draw those pixels over again with srcPixmap?
I guess, simple QPIxmap pixmap = srcPixmap should work (QPixmap will detach as
soon as you start the pixmapPainter).


> +	  
pixmapPainter.setCompositionMode(QPainter::CompositionMode_DestinationIn);
> +	   pixmapPainter.drawPixmap(0, 0, maskPixmap);
> +	   pixmapPainter.end();
> +	   painter->drawPixmap(offset, pixmap);

Brings me another question: do we need 'pixmap' at all? Can't we just do:

painter->save();
painter->setCompositionMode(QPainter::CompositionMode_Source);
painter->drawPixmap(offset, srcPixmap);
painter->setCompositionMode(QPainter::CompositionMode_DestinationIn);
painter->drawPixmap(0, 0, maskPixmap);
painter->restore();


>  void GraphicsLayerQt::setBackgroundColor(const Color& c)
>  {
> +    if (c == backgroundColor())
> +	   return;

Is this necessary for this patch or is it actually a separate optimization?



Otherwise, looks really good :)