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

[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 31523: part two, v2
https://bugs.webkit.org/attachment.cgi?id=31523&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Style:
	 inline uint32_t readUint32(SharedBuffer* data, int offset) const {
 50		return readUint32Helper(data, m_decodedOffset + offset);
 51	    }
Fight your google instincts!  fight, fight fight! ;)

Why this change?
 115	 RGBA32Buffer* buffer = &m_frameBufferCache.first();

I don't think the wrapping to 80c is really helpful in cases liek this:

8	  if (((m_headerOffset + m_infoHeader.biSize + SIZEOF_BITMASKS) <
 479		     (m_headerOffset + m_infoHeader.biSize))
 480		 || (m_imgDataOffset && (m_imgDataOffset <
 481		     (m_headerOffset + m_infoHeader.biSize +
SIZEOF_BITMASKS)))) {
 482		 setFailed();

or this:
	     m_bitMasks[i] &=
 511		     ((static_cast<uint32_t>(1) << m_infoHeader.biBitCount) -
1);

are we trying to merge these with something?  If not, then there is no need to
try and keep 80c wrapping.

Style:
137	    inline uint16_t readUint16(SharedBuffer* data, int offset) const {
 138		 return readUint16Helper(data, m_decodedOffset + offset);
 139	     }
 140 
 141	     inline uint32_t readUint32(SharedBuffer* data, int offset) const {

 142		 return readUint32Helper(data, m_decodedOffset + offset);
 143	     }

In general this looks fine.  Please address the above issues when landing.