[webkit-reviews] review denied: [Bug 58851] add incremental decoding to WebP decoder : [Attachment 90115] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Pascal Massimino
<pascal.massimino at gmail.com>'s request for review:
Bug 58851: add incremental decoding to WebP decoder
https://bugs.webkit.org/show_bug.cgi?id=58851

Attachment 90115: Patch
https://bugs.webkit.org/attachment.cgi?id=90115&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90115&action=review

Looks great.  Some minor comments below.

> Source/WebCore/ChangeLog:8
> +	   No new tests needed.

Normally we like a little more song and dance here about now incremental
decoding is good for the soul and difficult to test.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:87
>      const uint8_t* dataBytes =
>	   reinterpret_cast<const uint8_t*>(m_data->data());

This should be on one line.  There isn't an 80 col limit.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:100
> +    const bool dataComplete = isAllDataReceived();
> +    const int stride = width * bytesPerPixel;

WebKit isn't super agressive about using the const qualifier, but these are
probably fine.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:109
> +	   buffer.setHasAlpha(false);

I'd add a FIXME about changing this once WebP supports alpha.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:141
> +	    buffer.setStatus(ImageFrame::FrameComplete);

Should we assert some relation to dataComplete here?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:145
> +    } else {
> +	    return false; // I/O suspension.
>      }

No { } on these lines.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:36
> +typedef struct WebPIDecoder WebPIDecoder;

Should this typedef happen inside the WebCore namespace to avoid pollution?

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:51
> +    // incremental decoder, in case only partial data is available.

Please make this comment a complete sentence that starts with an initial
capital letter.