<html>
    <head>
      <base href="https://bugs.webkit.org/" />
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add APNG support"
   href="https://bugs.webkit.org/show_bug.cgi?id=17022#c35">Comment # 35</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add APNG support"
   href="https://bugs.webkit.org/show_bug.cgi?id=17022">bug 17022</a>
              from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=236444&action=diff" name="attach_236444" title="v07_apng_webkit.patch">attachment 236444</a> <a href="attachment.cgi?id=236444&action=edit" title="v07_apng_webkit.patch">[details]</a></span>
v07_apng_webkit.patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=236444&action=review">https://bugs.webkit.org/attachment.cgi?id=236444&action=review</a>

I'm fine with adding APNG support to WebKitGTK+. I don't know the APNG details, so I can't review this from that point of view (fortunately it includes layout tests), but I can comment about coding style and other general nits. Thanks for the patch!

<span class="quote">> Source/WTF/ChangeLog:3
> +        Added ENABLE(APNG)</span >

Use the bug title here

<span class="quote">> Source/WebCore/platform/image-decoders/ImageDecoder.h:169
> +        inline unsigned divide255(unsigned a)</span >

This could probably be static

<span class="quote">> Source/WebCore/platform/image-decoders/ImageDecoder.h:177
> +            if (!a)
> +                return;</span >

Shouldn't we initialize dest in this case? *dest = 0;

<span class="quote">> Source/WebCore/platform/image-decoders/ImageDecoder.h:201
> +                } else
> +                if (m_premultiplyAlpha) {</span >

This should be a single line.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:244
> +    , m_png(0)</span >

Use nullptr instead of 0 to initialize all pointer members.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:259
> +    , m_width(0)
> +    , m_height(0)</span >

Could this be an IntSize?

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:261
> +    , m_xOffset(0)
> +    , m_yOffset(0)</span >

And this IntPoint?

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:295
> +        return 0;</span >

nullptr.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:433
> +            } else
> +            if (colorType == PNG_COLOR_TYPE_GRAY) {</span >

This should be a single line.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:478
> +        m_gamma = (int)(gamma*100000);</span >

Use C++ style cast here and leave space around the *

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:568
> +#if ENABLE(APNG)
> +        if (!m_curFrame)
> +#endif</span >

So, it seems many things depend on m_curFrame, I wonder if we could move m_curFrame out of the #ifdef, but it will always be 0 for non APNG. This way we can avoid a lof of #ifdefs in the code I think.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:725
> +            return 1;</span >

So, what's this function supposed to return? It seems we are always retuning 1. If that's the case, I would make this function void and return 1 unconditionally in the static readChunks()

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:742
> +        for (size_t i = 0; i < m_frameCount; ++i)
> +            m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha);</span >

You could use a modern loop here 

for (auto& imageFrame : m_frameBufferCache)
    imageFrame.setPremultiplyAlpha(m_premultiplyAlpha);

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:744
> +    } else
> +    if (!memcmp(chunk->name, "fcTL", 4) && chunk->size == 26) {</span >

This should be a single line.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:785
> +    } else
> +    if (!memcmp(chunk->name, "fdAT", 4) && chunk->size >= 4) {</span >

This should be a single line

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:808
> +    double gamma;</span >

Move this where it's first needed.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:809
> +    int bitDepth = png_get_bit_depth(m_png, m_info);</span >

Ditto, this could be moved after the first if

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:849
> +        return; // Nothing to do.</span >

I don't think the comment is needed.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:880
> +    if (frameIndex >= frameCount())
> +        return;</span >

Could we check this before? I guess the frameRect computation doesn't affect this.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:882
> +    ImageFrame* const buffer = &m_frameBufferCache[frameIndex];</span >

Could this be const ImageFrame& instead of using a pointer?

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:935
> +    else
> +    if (!m_delayDen)</span >

This should be a single line

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:943
> +    else
> +    if (m_dispose == 1)</span >

Ditto.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:1022
> +    png_byte dataPNG[8] = {137, 80, 78, 71, 13, 10, 26, 10};
> +    png_byte datagAMA[16] = {0, 0, 0, 4, 103, 65, 77, 65};</span >

Could this be static?

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:1054
> +    png_byte dataIEND[12] = {0, 0, 0, 0, 73, 69, 78, 68, 174, 66, 96, 130};</span >

Ditto.

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:1078
> +    return 1;</span >

Why does this method returns int if it always returns 1?

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:31
> +#include "png.h"</span >

I guess this should be <png.h>

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:49
> +        virtual size_t frameCount() { return m_frameCount; }
> +        virtual int repetitionCount() const { return m_playCount-1; }</span >

These should be marked as override

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:112
> +        unsigned m_curFrame;</span >

m_currentFrame

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:116
> +        unsigned m_seqNum;</span >

m_sequenceNumber

<span class="quote">> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h:122
> +        unsigned m_delayNum;
> +        unsigned m_delayDen;</span >

What do Num and Den stand for here? Do not use abbreviations.

<span class="quote">> LayoutTests/ChangeLog:4
> +        Added APNG reftest.
> +        <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add APNG support"
   href="show_bug.cgi?id=17022">https://bugs.webkit.org/show_bug.cgi?id=17022</a></span >

Keep the bug title as the title of all changelogs of the same commit.</pre>
        </div>
      </p>
      <hr>
      <span>You are receiving this mail because:</span>
      
      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>