Bug 20945 - REGRESSION (r36628): Safari icon on error page is squished
Summary: REGRESSION (r36628): Safari icon on error page is squished
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Peter Kasting
Keywords: InRadar
: 20936 20944 (view as bug list)
Depends on:
Reported: 2008-09-19 11:53 PDT by mitz
Modified: 2008-09-22 14:49 PDT (History)
3 users (show)

See Also:

patch v1 (8.38 KB, patch)
2008-09-19 14:50 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
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.