Apparently (according to Wikipedia and Chrome's source code), icon files that have a width or height set to 0 in the header should have that direction set to 256. Bug 619048 contains a couple of examples of this. > 127-sized ICOs are very spottily supported, though; for example, the Windows shell supports displaying them just fine, but both IE and Windows Photo Viewer can't view them.

Yes, [1] is as authoritative as it gets about this. [1] http://blogs.msdn.com/b/oldnewthing/archive/2010/10/18/10077133.aspx

The reftests are a little bigger beause of 256x256, let me know if you want me to kick out a few of the tests on the BPP tests.

Comment on attachment 546611 [details] [diff] [review] Patch for 256x256 ICO decoder, encoder, and reftests > >+ // Obtains the width of the icon directory entry >+ PRUint32 GetRealWidth() const >+ { >+ return mDirEntry.mWidth == 0? 256 : mDirEntry.mWidth; >+ } >+ >+ // Obtains the height of the icon directory entry >+ PRUint32 GetRealHeight() const >+ { >+ return mDirEntry.mHeight == 0? 256 : mDirEntry.mHeight; >+ } >+ It would be better to just store the real height and do this translation at decode time.

It is an PRUint8 field in the BMP header so cannot store 256.

(In reply to comment #3) > Comment on attachment 546611 [details] [diff] [review] [review] > Patch for 256x256 ICO decoder, encoder, and reftests > Also, it seems like these png files could be much smaller: convert -strip ico-size-256x256-1bpp.png a.png -rw-r--r-- 1 jrmuizel staff 17197 18 Jul 16:20 ico-size-256x256-1bpp.png -rw-r--r-- 1 jrmuizel staff 7156 18 Jul 16:21 a.png

(In reply to comment #4) > It is an PRUint8 field in the BMP header so cannot store 256. Ah, I didn't realize we were trying to preserve the structure layout.

Oh nice about the file sizes. I'll apply a strip to those PNGs once Joe reviews and submit a new patch with any of his comments + those.

Same as last patch but with stripped (convert -strip) and optimized (optipng) PNG images.

Comment on attachment 546769 [details] [diff] [review] Patch for 256x256 ICO decoder, encoder, and reftests Review of attachment 546769 [details] [diff] [review]: ----------------------------------------------------------------- Other than formatting, looks good! ::: modules/libpr0n/decoders/nsICODecoder.cpp @@ +256,5 @@ > (mCurrIcon*sizeof(mDirEntryArray))) { > mCurrIcon++; > ProcessDirEntry(e); > + if (((e.mWidth == 0? 256 : e.mWidth) == PREFICONSIZE > + && (e.mHeight == 0? 256 : e.mHeight) == PREFICONSIZE && GetRealWidth/GetRealHeight, right? (Also && on prev line, as long as you're in here)

GetReadWidth/GetRealHeight operates on the dir entry that we end up using and assigning to mDirEntry. This part of code is in a loop going through each of the dir entry items, so it needs to be like so. I will fix the formatting though.

Also add a comment then :)

Same as last patch but rebased to work with the new series of patches.

Needed another rebasing due to review comments on BMP decoder patch.

Rebased patch to work with Bug 670466's and Bug 549468's latest patch.

Comment on attachment 550724 [details] [diff] [review] Rebased patch for 256x256 ICO decoder, encoder, and reftests Review of attachment 550724 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpr0n/encoders/ico/nsICOEncoder.h @@ +104,5 @@ > // or if no encoding options specified will use the default (PNG) > nsCOMPtr<imgIEncoder> mContainedEncoder; > > // These headers will always contain endian independent stuff > + // Don't trust the width and height of mICODirEntry directly, use accessors maybe specify the names of these accessors

Same as last patch which was r+ just a comment change, so just marking as r+

Rebased, will push to try when tree opens again.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=fcb3a88198ee

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/4da52874a049