Bug 154074 – [cmake] Consolidate CMake code related to image decoders.

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

[Konstantin Tokarev]

Common image decoder sources, includes and libs are moved to WebCore/ImageDecoders.cmake.

[Konstantin Tokarev]

Also, added include directories of libjpeg and libpng to WebCore_SYSTEM_INCLUDE_DIRECTORIES.

[Konstantin Tokarev]

Created attachment 270992 [details]
Patch

[Konstantin Tokarev]

Created attachment 270993 [details]
Patch

[Alex Christensen]

Comment on attachment 270993 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        * ImageDecoders.cmake: Added.

I don't have a strong opinion either way, but I know we've talked about making the WebCore directory less cluttered.  I think it might be better to put this in WebCore/platform.

> Source/WebCore/ImageDecoders.cmake:29
> +if (JPEG_FOUND)

I think these should remain unconditional.  We should have a hard failure if these aren't found.

[Konstantin Tokarev]

Comment on attachment 270993 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        * ImageDecoders.cmake: Added.
> 
> I don't have a strong opinion either way, but I know we've talked about making the WebCore directory less cluttered.  I think it might be better to put this in WebCore/platform.

I was afraid that WebCore/platform it may be lost between variouse source files, but I will move it there if you feel WebCore is not appropriate.

Alternatively, there can be WebCore/cmake (though AFAIK such directories are usually created to store macros and other helper modules)

>> Source/WebCore/ImageDecoders.cmake:29
>> +if (JPEG_FOUND)
> 
> I think these should remain unconditional.  We should have a hard failure if these aren't found.

WinCairo does not use find_package for libjpeg and libpng, therefore repspective variables are empty. I thought it is not a good practice to append empty variables to the lists.

[Konstantin Tokarev]

Comment on attachment 270993 [details]
Patch

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

>>> Source/WebCore/ImageDecoders.cmake:29
>>> +if (JPEG_FOUND)
>> 
>> I think these should remain unconditional.  We should have a hard failure if these aren't found.
> 
> WinCairo does not use find_package for libjpeg and libpng, therefore repspective variables are empty. I thought it is not a good practice to append empty variables to the lists.

BTW, hard failure is guaranteed by REQUIRED flag, set by ports in Options*.cmake when find_package is invoked.

[Alex Christensen]

(In reply to comment #6)
> Comment on attachment 270993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270993&action=review
> 
> >>> Source/WebCore/ImageDecoders.cmake:29
> >>> +if (JPEG_FOUND)
> >> 
> >> I think these should remain unconditional.  We should have a hard failure if these aren't found.
> > 
> > WinCairo does not use find_package for libjpeg and libpng, therefore repspective variables are empty. I thought it is not a good practice to append empty variables to the lists.
> 
> BTW, hard failure is guaranteed by REQUIRED flag, set by ports in
> Options*.cmake when find_package is invoked.
And JPEGImageDecoder.cpp is compiled unconditionally, so it won't link successfully if there is no library.  I guess these if (*_FOUND) checks are ok.

Just move the file to platform, then.

[Konstantin Tokarev]

Created attachment 271016 [details]
Patch

[Alex Christensen]

Comment on attachment 271016 [details]
Patch

r=me, let's wait for ews before committing.

[WebKit Commit Bot]

Comment on attachment 271016 [details]
Patch

Clearing flags on attachment: 271016

Committed r196394: <http://trac.webkit.org/changeset/196394>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.