[webkit-reviews] review granted: [Bug 35411] Clean up m_failed usage in open-source image decoders : [Attachment 54743] Part 2, v3

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Peter Kasting
<pkasting at google.com>'s request for review:
Bug 35411: Clean up m_failed usage in open-source image decoders
https://bugs.webkit.org/show_bug.cgi?id=35411

Attachment 54743: Part 2, v3
https://bugs.webkit.org/attachment.cgi?id=54743&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Please consider addressing the "Should we..." and compacting the ChangeLog
slightly through the use of ditto as mentioned below.

Thanks!

 
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 58541)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,37 @@
> +2010-04-29  Peter Kasting  <pkasting at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Make all image decoders set the "failed" bit if an image could not
be
> +	   completely decoded, but no more data is coming.  The ICO and BMP
> +	   decoders already did this.
> +	   https://bugs.webkit.org/show_bug.cgi?id=35411
> +
> +	   "Failed" does not cause the image to not be displayed, it simply
causes
> +	   us to not bother to try to decode again if future requests are made,
and
> +	   for some decoders, lets the decoder clean up some of its temporary
> +	   objects.
> +	   
> +	   No layout tests because this does not change the visible output of
decoding in any way.
> +
> +	   * platform/image-decoders/gif/GIFImageDecoder.cpp:
> +	   (WebCore::GIFImageDecoder::frameComplete): Return whether the frame
could be marked as complete.
> +	   (WebCore::GIFImageDecoder::decode): read() will mark decode failure
itself, so the only additional failure case is when read() needs more data (and
thus returns false) and no more is coming.
> +	   * platform/image-decoders/gif/GIFImageDecoder.h:
> +	   * platform/image-decoders/gif/GIFImageReader.cpp:
> +	   (GIFImageReader::do_lzw): Instead of returning true for buffer
underrun and false for failure, return false for both and set the failure flag
on failure.
> +	   (GIFImageReader::read): Instead of returning true for buffer
underrun and false for failure, return false for both and set the failure flag
on failure.

"Ditto." works instead of repeating the last phrase exactly.

Thanks for the notes. This helps me understand what you are doing better (and I
unfortunately kept reading "setFailed()" as "failure()" which was my other
source of confusion).



> Index: WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp
> +bool GIFImageDecoder::frameComplete(unsigned frameIndex, unsigned
frameDuration, RGBA32Buffer::FrameDisposalMethod disposalMethod)
>  {
>      // Initialize the frame if necessary.  Some GIFs insert do-nothing
frames,
>      // in which case we never reach haveDecodedRow() before getting here.
>      RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];
>      if ((buffer.status() == RGBA32Buffer::FrameEmpty) &&
!initFrameBuffer(frameIndex))
> -	   return;
> +	   return false; // initFrameBuffer() has already called setFailed().

Should we "assert(failed());" here (before the return and the comment could be
right after the assert)?


> Index: WebCore/platform/image-decoders/gif/GIFImageReader.cpp
>	   // Even though suffix[] only holds characters through suffix[avail -
1],
>	   // allowing code >= avail here lets us be more tolerant of malformed

> @@ -324,7 +326,7 @@ bool GIFImageReader::do_lzw(const unsign
>	   code = prefix[code];
>  
>	   if (stackp == stack + MAX_BITS)
> -	     return false;
> +	     return clientptr ? clientptr->setFailed() : false;

I kept reading this as possibly returning two different values but it always
returns false.


> +	 if (!do_lzw(q))
> +	   return false; // If do_lzw() encountered an error, it has already
called
> +			 // clientptr->setFailed().


Should we "assert(failed());" here (before the return and the comment could be
right after the assert)?

> +	   if (clientptr && frame_reader &&
!clientptr->frameComplete(images_decoded - 1, frame_reader->delay_time,
frame_reader->disposal_method))
> +	     return false; // frameComplete() has already called
> +			   // clientptr->setFailed().

Ditto.