[webkit-reviews] review denied: [Bug 80400] [Qt] Set WebCore imagedecoders as default, add fallback to QImageDecoder : [Attachment 131330] proposed patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 80400: [Qt] Set WebCore imagedecoders as default, add fallback to
QImageDecoder
https://bugs.webkit.org/show_bug.cgi?id=80400

Attachment 131330: proposed patch
https://bugs.webkit.org/attachment.cgi?id=131330&action=review

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


Nice, this is a lot simpler. I think only the image storage is left as an
issue. A few comments below.

> Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:238
> +// The image must not have format 8888 pre multiplied...

It is unfortunately not very clear from this comment what the problem is. Could
you elaborate a bit more? Is there an ASSERT we should use in the function
below?

> Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:244
> +void ImageFrame::setImage(const QImage& image)
> +{
> +    m_image = image;
> +    m_size = image.size();
> +    m_hasAlpha = image.hasAlphaChannel();
> +}

I suppose the answer to my m_bytes question lies here. Perhaps we can make
m_bytes point to the image's data and somehow make sure the vector doesn't take
ownership? Alternatively perhaps we can make m_bytes our _real_ storage, get
rid of m_image as a member and when using the QImageReader construct a
temporary QImage around the m_bytes data that we pass to the
QImageReader::read(QImage*) function.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:143
> -#elif PLATFORM(QT) && USE(QT_IMAGE_DECODER)
> -	       m_image = m_pixmap.toImage();
> -	       m_pixmap = QPixmap();
> -	       return reinterpret_cast_ptr<QRgb*>(m_image.scanLine(y)) + x;
>  #else
>	       return m_bytes + (y * width()) + x;

Hmm, what about the qt image decoder case? Won't this break if m_image is set
but m_bytes isn't?