This bug is tracking adjusting the ICO decoder to be non-incremental (since being incremental on a 16x16 icon is rather pointless anyway) and is also tracking ensuring that the alpha info is set first.

This patch stops using the bitmap info header width/height fields in favor of the icon dir entry width/height fields. What I'm seeing with some favicons on the net is that the width/height fields are sometimes garbage in the bitmap info header. It appears that they're relying on the decoder only using the icon dir entry width/height. While I'm sure the ico files are technically malformed, this makes the ICO decoder more robust in that it can successfully view those files.

Looking for r/sr on this patch.

Comment on attachment 56454 [details] [diff] [review] Cleanup. r=pavlov

Comment on attachment 56454 [details] [diff] [review] Cleanup. sr=jst

Comment on attachment 56699 [details] [diff] [review] Non-incremental decoder, sets alpha before image data. r=pavlov

Comment on attachment 56699 [details] [diff] [review] Non-incremental decoder, sets alpha before image data. >Index: decoders/bmp/nsICODecoder.cpp >+ mFrame->GetImageBytesPerRow(&bpr); >+ for (PRUint32 i = 0; i < mDirEntry.mHeight; i++) { >+ PRUint32 offset = bpr*i; >+ mFrame->SetImageData(mDecodedBuffer+offset, bpr, offset); >+ } how about this instead: for (PRUint32 offset = 0, i = 0; i < mDirEntry.mHeight; ++i, offset += bpr) mFrame->SetImageData(mDecodedBuffer + offset, bpr, offset); >+ for (PRUint32 i = 0; i < mDirEntry.mHeight; i++) { >+ PRUint32 offset = bpr*i; >+ mFrame->SetAlphaData(mAlphaBuffer+offset, bpr, offset); >+ } likewise, here.

strength reduce as darin suggested, and sr=waterson.

Fixed.

If reduction in strength is appropriate, why not also eliminate the induction variable (i)? for (PRUint32 offset = 0, limit = mDirEntry.mHeight * bpr; offset < limit; offset += bpr) mFrame->SetImageData(mDecodedBuffer + offset, bpr, offset); /be