[webkit-reviews] review denied: [Bug 53455] Update ImageDecoder as suggested on webkit-dev : [Attachment 80841] Larger patch v1.1

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Peter Kasting
<pkasting at google.com>'s request for review:
Bug 53455: Update ImageDecoder as suggested on webkit-dev
https://bugs.webkit.org/show_bug.cgi?id=53455

Attachment 80841: Larger patch v1.1
https://bugs.webkit.org/attachment.cgi?id=80841&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80841&action=review

Sorry to r- over misc issues in comments but since that is what this patch is
about it seems appropriate.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:52
> +    // ImageFrame represents the decoded image data.  This buffer is what
all

Since you're fixing up the comments consider adjusting the ones that you fix
have only one space after periods which is typical in WebKit (although not in
the style guide yet because I haven't had a chance to update it).

> Source/WebCore/platform/image-decoders/ImageDecoder.h:115
> +	   // (This pointer will be owned by BitmapImage and freed in

Is this last sentence needed? It could be owned by and freed by any code and
this code would not be affected. 

Also you added an open paren but no close paren.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:191
> +	   IntSize m_size; // The size of the buffer.  This should be the same
as

Would m_bufferSize be a better name? (and then the first sentence seems
redundant).

> Source/WebCore/platform/image-decoders/ImageDecoder.h:196
> +	   IntRect m_rect; // The rect of the original specified frame within
the

m_originalFrameRect? (I'm not sure if this rename helps or not. Just an idea.)

> Source/WebCore/platform/image-decoders/ImageDecoder.h:226
> +	   // Factory function to create an appropriate ImageDecoder.  Returns
0 if

Does this first sentence add anything?

It feels like the thing missing from this comment is that the caller need to
take ownership of the returned value (but changing it to PassOwnPtr in another
change will fix that w/o a comment).

> Source/WebCore/platform/image-decoders/ImageDecoder.h:243
> +	   // Lazily-decodes enough of the image to get the size (if possible).


I don't understand this comment as this function doesn't lazily decode
anything.  Perhaps it should be removed.

It just does "return !m_failed && m_sizeAvailable;9"

> Source/WebCore/platform/image-decoders/ImageDecoder.h:262
> +	   // frame.

I'm trying to shorten this comment. How about

"Returns the size of a particular frame which may vary for formats like ICO,
where each frame represents a different icon.  Notably, for GIF, the size is
constant because decoding makes all frames the same size."

Still a bit long but slightly shorter.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:268
> +	   // Sets the image size.  Returns whether the size is legal (i.e. not


It looks like the first sentence may go away.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:280
> +	   // Lazily-decodes enough of the image to get the frame count (if

This base implementation doesn't appear to lazily decode anything.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:287
> +	   // ImageDecoder-owned pointer.

Why is this "Decodes as much of the requested frame as possible" notable?

I would guess that the function would decode the frame buffer at the given
index due to its name. Is it notable because it may not decode the whole frame?


> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
> +	   // frames.  In practice this is used on large animated GIFs to save

It feels like you could remove everything before "In practice" (and I'm not
sure if that is needed either as long as I can grep the code for it because
usage may change).