|Summary:||REGRESSION (r36628): Safari icon on error page is squished|
|Component:||Images||Assignee:||Peter Kasting <pkasting>|
|Severity:||Normal||CC:||amfr, balli.h, pkasting|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description mitz 2008-09-19 11:53:18 PDT
The Safari icon on Safari error pages is squished. To reproduce: open Safari and go to <http://safaricantfindtheserver.com/>. It looks like r36628 introduced a change that causes the current frame in the image to advance to 4. This in turn confuses the code in BitmapImage::draw, which uses the size of frame 4 (16x16) while the caller passes a 512x512 source rectangle, based on frame 0.
Comment 2 mitz 2008-09-19 12:01:56 PDT
*** Bug 20944 has been marked as a duplicate of this bug. ***
Comment 3 Peter Kasting 2008-09-19 14:42:16 PDT
I can't repro on Safari for Windows, which uses a PNG rather than an .icns. However, I think I have a fix for this anyway. r36628 is blameless here; it just allowed animated images to actually animate. The real bug was the earlier change of mine that made us think this was an animated image, because shouldAnimate() was checked before m_repetitionCount got set.
Comment 4 Peter Kasting 2008-09-19 14:50:24 PDT
Created attachment 23582 [details] patch v1 I can't test that this actually fixes the bug, but I bet it will; please try it out. The attempted fix here is to be more careful about getting/setting m_repetitionCount, similar to how we already do with other members that mirror members on m_source. Don't check it unless it's actually set. I eliminated m_animatingImageType, since it was just equivalent to (m_repetitionCount == cAnimationNone) and thus unnecessary. Instead of a simple bool m_haveRepetitionCount like the rest of the members on this class do I had to use a tristate enum m_repetitionCountStatus, to handle the way we recheck the repetition count at the end of decoding the image.
Comment 5 Mark Rowe (bdash) 2008-09-19 15:31:22 PDT
*** Bug 20936 has been marked as a duplicate of this bug. ***
Comment 7 mitz 2008-09-20 15:35:23 PDT
(In reply to comment #6) > The r36628 change was rolled out in r36702. So should this bug be resolved?
Comment 8 Peter Kasting 2008-09-22 14:49:57 PDT
Closing, since the original patch that caused this regression has now been backed out.