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

[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 option to
QImageDecoder
https://bugs.webkit.org/show_bug.cgi?id=80400

Attachment 130632: proposed patch, chrome-ews fix
https://bugs.webkit.org/attachment.cgi?id=130632&action=review

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


This is a good start :) A few comments, in general I think there's a
possibility of reducing the complexity of this patch a lot.

> Source/WebCore/ChangeLog:7
> +	   however I add a new macro (called
WTF_USE_FALLBACK_TO_QT_IMAGE_DECODER) which allows you to compile with

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.

> 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.

> 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.

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 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?