[webkit-reviews] review denied: [Bug 113124] Add animation support for WebP images : [Attachment 307748] Patch

[bugzilla-daemon at webkit.org]

Carlos Alberto Lopez Perez <clopez at igalia.com> has denied Olivier Blin
<olivier.blin at softathome.com>'s request for review:
Bug 113124: Add animation support for WebP images
https://bugs.webkit.org/show_bug.cgi?id=113124

Attachment 307748: Patch

https://bugs.webkit.org/attachment.cgi?id=307748&action=review




--- Comment #11 from Carlos Alberto Lopez Perez <clopez at igalia.com> ---
Comment on attachment 307748
  --> https://bugs.webkit.org/attachment.cgi?id=307748
Patch

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

I was unable to test this because it currently fails to build (on WebKitGTK+
trunk of today=r216073).
Can you try to fix the comments below and upload a new version? Thanks

> Source/WebCore/ChangeLog:23
> +	   - layout and pixel tests

I think at least one layout test is needed for the patch to land. So please,
add some layout test on the next version of the patch.
I think there are some examples of how this can be done on the old patch
attached on this bug.

Since the Mac port doesn't use this image decoders, I think any test added
should be marked as skipped on the general TestExpectation file
<LayoutTests/TestExpectations> and then marked to pass (overriding the general
expectation) on the GTK test expectation file
<LayoutTests/platform/gtk/TestExpectations>

> Source/WebCore/ChangeLog:24
> +	   - remove support of old WebP versions in CMake files (all distros
with WebKitGTK ship a recent enough WebP with demuxer enabled)

Our current minimum baseline supported distribution (Debian 8) ships with this.
So I think is fine to assume demuxer should be available.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:57
> +    , m_formatFlags(0)
> +    , m_frameBackgroundHasAlpha(false)
> +    , m_demux(0)
> +    , m_demuxState(WEBP_DEMUX_PARSING_HEADER)
> +    , m_haveAlreadyParsedThisData(false)
> +    , m_repetitionCount(WebCore::RepetitionCountOnce)
> +    , m_decodedHeight(0)

Where are this class variables defined?

This fails to build for me: http://sprunge.us/PMGC
The EWS was happy, so perhaps something changed since the patch was uploaded?

> Source/cmake/FindWebP.cmake:34
>  pkg_check_modules(PC_WEBP QUIET libwebp)
>  

Maybe this could be:
pkg_check_modules(PC_WEBP QUIET libwebp libwebpdemux)

So below you don't need to append to those arrays, neither change the variable
names on find_path() and find_library()

This means we would require webp with demuxer support built-in or we will skip
to build any webp support at all.