[webkit-reviews] review denied: [Bug 26460] Cross-platform ICO decoder should return all icons in file as separate frames, sorted by decreasing quality : [Attachment 34031] Return multiple frames from ICO decoder

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied 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 34031: Return multiple frames from ICO decoder
https://bugs.webkit.org/attachment.cgi?id=34031&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
When is "*i" going to be NULL?
     for (BMPReaders::iterator i(m_BMPReaders.begin());
 69	     i != m_BMPReaders.end(); ++i) {
 70	    if (*i)
 71		(*i)->setData(data);
7172	 }

Seems rather strange to have readers in your list which will be null.

I believe the style is m_bmpReaders instead of m_BMPReaders.  and m_pngDecoders
instead of m_PNGDecoders.

Seems this shoudl use early-return to avoid the long indented block:
 132	 if (m_PNGDecoders[index]) {

 133	     // The size calculated inside the PNGImageDecoder had better match
the
 134	     // one in the icon directory.

probably should be:
// Fail if the size the PNGImageDecoder calculated does not match the size in
the directory

It seems IconDirectoryEntry should have a size() accessor which returns a
IntSize() given that you have to convert it to an IntSize more than once.  Or
at least that:
138		if ((pngSize.width() != dirEntry.bWidth)
 139		     || (pngSize.height() != dirEntry.bHeight)) {
would be simpler if it had such a function.

Seems another early return is in order:
 169	 if (m_PNGDecoders[index]) {
 170	     // Copy out PNG data to a separate vector and send to the PNG
decoder.

We don't have any column wrapping rule:
199	return (m_decodedOffset >=
 200		 (sizeOfDirectory + (m_dirEntries.size() * sizeOfDirEntry)))
 201	     || processDirectoryEntries();

Seems this block could instead be a helper function "determineImageType":
208	if (!m_BMPReaders[index] && !m_PNGDecoders[index]) {
 209	     // Image type unknown.
maybe a separate function is not clearer, not sure.

Doesn't Vector have a VectorTraits or some such to specify a default value?  Or
isnt' there a .fill()?	 I'm surprised you have to use this VectorInitializer
stuff, but maybe that's the right way...

No wrapping needed:
3     for (IconDirectoryEntries::iterator i(m_dirEntries.begin());
 284	      i != m_dirEntries.end(); ++i)


It looks like the PNGDecoders type is never used.

Seems odd to use an iterator for one here and not the other:
68     for (BMPReaders::iterator i(m_BMPReaders.begin());
 69	     i != m_BMPReaders.end(); ++i) {
 70	    if (*i)
 71		(*i)->setData(data);
7172	 }
 73	for (size_t i = 0; i < m_PNGDecoders.size(); ++i)
 74	    setDataForPNGDecoderAtIndex(i);

if the iterator is not used, then the BMPReaders typedef is never needed.

r- for the nits above.

Also, I look at the review queue every day, but I only click on bugs which
Safari thinks are un-visited.  Occasionally I look through the whole queue
(like I am doing tonight).  So I won't notice your patches if you keep
attaching them to old bugs. :)	WebKit seems to be of many minds about whether
to re-use bugs or not. :)  (I go either way depending on the patch.)