[Webkit-unassigned] [Bug 58851] add incremental decoding to WebP decoder

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58851


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #90115|review?                     |review-
               Flag|                            |




--- Comment #2 from Adam Barth <abarth at webkit.org>  2011-04-18 16:38:24 PST ---
(From update of attachment 90115)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.