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

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has asked  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 Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #7)
> I think it should be our goal for the Qt port to reduce the number of
configurations of features we have to maintain. In this case I don't think we
do need this as another option and I suggest that we simply do image decoding
this way #if PLATFORM(QT). This should also make your patch slightly simpler.

I totally agree with you, I removed the newly added (FALLBACK) and the old
(WTF_USE_QT_IMAGE_DECODER) macro.
 
> > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149
> > +#if PLATFORM(QT)
> > +	 m_pixmap = QPixmap();
> > +	 m_image = QImage();
> > +#endif
> 
> I don't think that this block of code is needed, given that this is a
constructor and that m_pixmap and m_image already have proper default
constructors.

Yes, you're right. I removed these.

> > Source/WebCore/platform/image-decoders/ImageDecoder.h:338
> > +	     virtual bool isImageDecoderQt() { return false; }
> 
> This should be const, but another option would be to simply take the existing
booleans in the class, compress them into a bitfield and add another bit
indicating if the constructed decoder is a Qt decoder or not, removing the need
for a virtual function.

I find a simpler way to solve this. Do you like it?

> In an ideal world however the Qt image decoder would simply be _another_
ImageDecoder subclass that is instantiated, just like the png/jpeg/bmp/ico/etc.
decoders, with as few #ifdefs in ImageDecoder.cpp as they are needed for the
others (i.e. zero).

I try to reduce the number of ifdefs, but I left some, so this patch may need
further iterations.

> I suspect that one reason for this used to be that we had this _need_ to load
images into QPixmaps, where pixmaps used to be potentially server-side objects.
Since this "constraint" has gone, perhaps it is possible to operate on the same
data structures like the other decoders and avoid extra #ifdefs here?

I removed the qpixmap from ImageDecoder, so currently we use only qimage for
QImageDecoder. asNewNativeImage() still returns with NativeImagePtr. Modifying
NativeImagePtr from QPixmap* to QImage* would be a bigger task. What do you
think of it?

Notice: this patch adds build depedency for libpng-dev and libjpeg-dev, which
has been installed on our EWS by Ossy already.