Bug 20945

Summary: REGRESSION (r36628): Safari icon on error page is squished
Product: WebKit Reporter: mitz
Component: ImagesAssignee: Peter Kasting <pkasting>
Status: RESOLVED FIXED    
Severity: Normal CC: amfr, balli.h, pkasting
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
patch v1 none

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 1 mitz 2008-09-19 11:53:47 PDT
<rdar://problem/6232943>
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 6 Timothy Hatcher 2008-09-20 15:31:28 PDT
The r36628 change was rolled out in r36702.
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.