[webkit-reviews] review granted: [Bug 47512] Add support for decoding WebP image : [Attachment 70801] updated patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Pascal Massimino
<pascal.massimino at gmail.com>'s request for review:
Bug 47512: Add support for decoding WebP image
https://bugs.webkit.org/show_bug.cgi?id=47512

Attachment 70801: updated patch
https://bugs.webkit.org/attachment.cgi?id=70801&action=review

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

Mostly a bunch of nits.  I can clean them up as I land the patch.  I'm not
super cheesed about the sniffing code, but I have a patch out for review that
makes this code more sane.  I'll just roll a more sane version into that patch.


> WebKit/WebCore/platform/image-decoders/ImageDecoder.cpp:86
> +	   unsigned length =
> +	       copyFromSharedBuffer(header, webpExtraMarker, data,
> +				    webpExtraMarkeroffset);

This should all be on one line.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:92
> +    // FIXME: support for progressive decoding.

Add support for ...

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:108
> +    if (!WebPDecodeBGRInto(data_uint8, data_size,
> +			      &rgb[0], height * stride, stride))

One line.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:118
> +	       buffer.setRGBA(x, y,
> +			      src[bytesPerPixel * x + 2],
> +			      src[bytesPerPixel * x + 1],
> +			      src[bytesPerPixel * x + 0],
> +			      0xff);

One line.  WebKit never met a line that was too long.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:36
> +namespace WebCore {

Missing blank line after this line.  Also, code inside a namespace should not
be indented.

> WebKit/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h:47
> +	   // return false in case of decoding failure.       

return -> Returns

> WebKit/LayoutTests/fast/images/webp-image-decoding.html:3
> +<img src="resources/test.webp">

You'll probably need to add this test to a bunch of skipped lists since very
few ports have support for this library.