[webkit-reviews] review denied: [Bug 37804] Image shadow doesn't work in Qt : [Attachment 53711] ImageShadowPatch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied qi <qi.2.zhang at nokia.com>'s
request for review:
Bug 37804: Image shadow doesn't work in Qt
https://bugs.webkit.org/show_bug.cgi?id=37804

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>


> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (ctxt->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +	   QImage shadowImage =
image->toImage().convertToFormat(QImage::Format_ARGB32_Premultiplied);
> +	   FloatRect shadowM(dst);
> +
> +	   shadowM.move(shadowSize.width(), shadowSize.height());
> +	   // set image color
> +	   for (int x = 0; x < shadowImage.height(); x++) {
> +	       for (int y = 0; y < shadowImage.width(); y++) {
> +		   if (qRed(shadowImage.pixel(x, y)) ||
qGreen(shadowImage.pixel(x, y)) || qBlue(shadowImage.pixel(x, y)))

Why do you do a conversion to ARGB_Premultiplied?

Also it seems strange to carefully test the RGB components individually instead
of just masking
out the alpha component and checking the remainder for != 0.

The most worrying part about this code however is that it means we have to do a
conversion from a pixmap
to an image on every paint. That means on systems where QPixmap is implemented
as surface that lives in
video memory or in a different process (X11) this is a very expensive
operation.

> +		       shadowImage.setPixel(x, y, shadowColor.rgb());

Please don't call setPixel() in an inner loop. As the documentation of
QImage::setPixel explains, this is a very
expensive function to call.