Bug 87841

Created attachment 144778 [details] proposed patch

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

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

+ 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 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 */

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.

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

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

Created attachment 145576 [details] proposed patch Config-test + HAVE->USE modification.

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