Bug 87841 – [Qt] Add config tests for WEBP imagedecoder

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Zoltan Horvath]

Add config tests for WEBP, enable building of WEBP imagedecoder if WEBP library is installed on the system.

[Zoltan Horvath]

Created attachment 144778 [details]
proposed patch

[Simon Hausmann]

Comment on attachment 144778 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144778&action=review

> Source/WebCore/WebCore.pri:246
> +    contains(config_test_libwebp, yes) {
> +        DEFINES += WTF_USE_WEBP=1

In an ideal world WTF_USE_WEBP=1 would be consistent with HAVE_LIBJPEG.

I guess the latter (jpeg/png) is considered really essential for web browsing, while webp is optional (and the define is used by others, too).

[Zoltan Horvath]

Created attachment 144793 [details]
patch

If you wish I can split the patch to 2 parts. (USE-HAVE change, configtest)
What is your opinion?

[noel gordon]

+        DEFINES += WTF_USE_WEBP=1
+        DEFINES += HAVE_WEBP=1

Would make for a smaller change.  We've used USE(XXXXX) everywhere, USE(ICCJPEG), USE(SKIA), USE(FOOBAR), and so on.  What does HAVE buy us?

[Zoltan Horvath]

(In reply to comment #4)
> +        DEFINES += WTF_USE_WEBP=1
> +        DEFINES += HAVE_WEBP=1
> 
> Would make for a smaller change.  We've used USE(XXXXX) everywhere, USE(ICCJPEG), USE(SKIA), USE(FOOBAR), and so on.  What does HAVE buy us?

In my opinion using of these decoders is more depending on that we HAVE the required libraries than we just want to USE. So if we have them, then we can always use them. It indicates me that using of HAVE is more natural.

From platform.h:

/* HAVE() - specific system features (headers, functions or similar) that are present or not */

/* USE() - use a particular third-party library or optional OS service */

[noel gordon]

So HAVE is for system header files it seems. WTF/wtf has examples:

ack HAVE Source/WTF/wtf/DateMath.cpp
89:#if HAVE(ERRNO_H)
93:#if HAVE(SYS_TIME_H)
97:#if HAVE(SYS_TIMEB_H)
373:#if HAVE(TM_GMTOFF)
376:#if HAVE(TM_ZONE)
380:#if HAVE(TIMEGM)

> /* USE() - use a particular third-party library or optional OS service */

For libwebp, libjpeg, libpng (third-party libs) etc, this comment suggests that it should be USE.  Of course we must have a lib available :) but that doesn't mean we USE it :) 

Don't mean to hold you up here Zoltan. I just found HAVE for a third party lib a little unusual based on my reading of WebKit code.

[Zoltan Horvath]

(In reply to comment #6)

> For libwebp, libjpeg, libpng (third-party libs) etc, this comment suggests that it should be USE.  Of course we must have a lib available :) but that doesn't mean we USE it :) 
> 
> Don't mean to hold you up here Zoltan. I just found HAVE for a third party lib a little unusual based on my reading of WebKit code.

Thanks for collecting the examples. I see your point! Our goal is to be consistent, so I'm okay with the USE macro also. If we made a decision to use that, then I'll update the patch. :)

I look forward to hear other's opinions also.

Simon, Tor Arne?

[Simon Hausmann]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > For libwebp, libjpeg, libpng (third-party libs) etc, this comment suggests that it should be USE.  Of course we must have a lib available :) but that doesn't mean we USE it :) 
> > 
> > Don't mean to hold you up here Zoltan. I just found HAVE for a third party lib a little unusual based on my reading of WebKit code.
> 
> Thanks for collecting the examples. I see your point! Our goal is to be consistent, so I'm okay with the USE macro also. If we made a decision to use that, then I'll update the patch. :)
> 
> I look forward to hear other's opinions also.
> 
> Simon, Tor Arne?

I agree with Noel, let's use USE() and set it in the build system if it turns out that we also _have_ it. That still gives the option of _not_ using it even if the library is present.

In other words: It seems HAVE() should be used when there's no doubt that all about whether we should use the feature if it's present in the system or not. USE() on the other hand is for things that _are_ optional, and that is the case here as well as with the other image decoders.

[Zoltan Horvath]

Created attachment 145576 [details]
proposed patch

Config-test + HAVE->USE modification.

[Zoltan Horvath]

Committed r119977: <http://trac.webkit.org/changeset/119977>