[webkit-reviews] review granted: [Bug 26460] Cross-platform ICO decoder should return all icons in file as separate frames, sorted by decreasing quality : [Attachment 31526] part three

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Peter Kasting <pkasting at google.com>'s
request for review:
Bug 26460: Cross-platform ICO decoder should return all icons in file as
separate frames, sorted by decreasing quality
https://bugs.webkit.org/show_bug.cgi?id=26460

Attachment 31526: part three
https://bugs.webkit.org/attachment.cgi?id=31526&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I assume BMPs are always single-frame?	We should have a comment to that effect
here:
 RGBA32Buffer* BMPImageDecoder::frameBufferAtIndex(size_t index)
 70 {
 71	if (index)
 72	    return 0;
 73 

Why do we have to pre-allocate the space:
74     if (m_frameBufferCache.isEmpty())
 75	    m_frameBufferCache.resize(1);

Oh, I guess because we're grabbing it on the next line:
RGBA32Buffer* buffer = &m_frameBufferCache.first();

Still seems awkward somehow.

We generally try to think of more descriptive names than "Helper".

It seems strange that we have to do all these manual calls to set up the
BMPImageReader:
1     if (!m_reader) {
 102	     m_reader.set(new BMPImageReader(this, m_decodedOffset,
imgDataOffset,
 103					     false));
 104	     m_reader->setData(m_data.get());
 105	 }
 106 
 107	 if (!m_frameBufferCache.isEmpty())
 108	     m_reader->setBuffer(&m_frameBufferCache.first());
 109 
 110	 return m_reader->decodeBMP(onlySize);

I would think we could implement a virtual interface and pass "this" to the
BMPReader and it could grab all this appropriate data itself?

processFileHeader should take a reference instead of a pointer.  There seems to
be little value in using a pointer, especially since at the callsite you do:
&& !processFileHeader(&imgDataOffset))

The arg name "data" doesn't really add much here:
 46	    virtual void setData(SharedBuffer* data, bool allDataReceived);

getInfoHeaderSize is poorly named.  It does not actually return the size.  It
should be something like updateInfoHeaderSize or calculateInfoHeaderSize or
cacheInfoHeaderSize (You don't have to change it here, but next time you're
cleaning up this code...)

m_parent seems like kinda a funny name for the owning decoder.	I guess I would
have called it m_imageDecoder or m_owningDecoder maybe?  m_parent is fine...
just initially confusing.

When you ask m_parent->size() aren't you getting the size of the first frame? 
I guess all frames are the same size?  Is this true of ICO files too?

I think it's neat that setFailed() returns false, but it is a litlte confusing
that in other places of the code we don't use:
return setFailed();

Is it possible/expected that the malloc of any of these framebuffers
would/should fail?  Are there really large BMPs out there we need to be aware
of?  I believe fastMalloc will just abort() if it fails, which is fine.

confused why your'e not setting m_headerOffset anymore?
      m_decodedOffset = m_headerOffset = m_dirEntry.dwImageOffset;
 214	 m_decodedOffset = m_dirEntry.dwImageOffset;

It seems you should either remove or rename BMPImageReader::size() to avoid
confusion with m_parent->size()?

The 80c thing still seems silly to me.	Since we don't wrap to 80c in other
parts of WebCore.


This change looks great!  I think that the management of buffers between
ICODecoder and BMPImageReader is a little too manual/error-prone.  And the
"Helper" name is silly, but I can't seem to come up with a better one.	I like
that you split out the "Helper" code into another function, although you might
not have needed to if the buffer management was simpler.

I don't really need to see this another time.  Please read through and deal
with the nits above, but you're good to go.

I look forward to seeing these classes use things like IntPoint and IntSize :)