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

mitz
Reported 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.
Attachments
patch v1 (8.38 KB, patch)
2008-09-19 14:50 PDT, Peter Kasting 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
mitz
Comment 1 2008-09-19 11:53:47 PDT
<rdar://problem/6232943>
mitz
Comment 2 2008-09-19 12:01:56 PDT
*** Bug 20944 has been marked as a duplicate of this bug. ***
Peter Kasting
Comment 3 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.
Peter Kasting
Comment 4 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.
Mark Rowe (bdash)
Comment 5 2008-09-19 15:31:22 PDT
*** Bug 20936 has been marked as a duplicate of this bug. ***
Timothy Hatcher
Comment 6 2008-09-20 15:31:28 PDT
The r36628 change was rolled out in r36702.
mitz
Comment 7 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?
Peter Kasting
Comment 8 2008-09-22 14:49:57 PDT
Closing, since the original patch that caused this regression has now been backed out.
Note You need to log in before you can comment on or make changes to this bug.