Closed Bug 192631 Opened 17 years ago Closed 12 years ago

Animated images stop animating after Print / Print Preview

Categories

(Core :: Printing: Output, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: smontagu, Assigned: martijn.martijn)

References

()

Details

(Keywords: regression, testcase)

Attachments

(1 file, 5 obsolete files)

My fix for bug 182259 seems to have been too heavy-handed: after print preview
of a page with animated images, the images stop animating in the browser.

Steps to reproduce: load http://www.burlco.info/moz/previmg/ or any page with
animated images
Print Preview - the images stop animating
Change from Portrait to Landscape and check that the images are still not
animated (this was bug 133808)
Close Print Preview

Expected results: animation should restart.
Actual results: it doesn't.
Blocks: 119597
Keywords: regression
test URL no longer exists
*** Bug 353468 has been marked as a duplicate of this bug. ***
Severity: normal → minor
OS: Windows 2000 → All
Hardware: PC → All
Summary: Animated images stop animating after Print Preview → Animated images stop animating after Print / Print Preview
1. visit http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes (a gif)
2. print preview (or print)

animation stops.
animation begins where it left off if you do a page reload/refresh
Blocks: 182259
Severity: minor → normal
Component: Print Preview → Printing
Keywords: testcase
QA Contact: sujay → printing
Attached patch patch (obsolete) — Splinter Review
This seems to fix it.
Attachment #308347 - Flags: review?(smontagu)
Comment on attachment 308347 [details] [diff] [review]
patch

Ah, sorry, it only seems to fix the print preview case.
It doesn't fix the print case.
Attachment #308347 - Flags: review?(smontagu)
Attached patch patch (obsolete) — Splinter Review
Ok, this duplicated code is probably too lame. I guess I should probably put it in a special function named ResetAnimationMode or something?
Or is the whole idea wrong?
Attachment #308352 - Flags: review?(smontagu)
Comment on attachment 308352 [details] [diff] [review]
patch

(In reply to comment #6)

> Ok, this duplicated code is probably too lame. I guess I should probably put it
> in a special function named ResetAnimationMode or something?

Yes, I think so. Having said that, I think you could find a better reviewer for this than me, like someone who's worked on printing code sometime in the last 5 years :)
Attachment #308352 - Flags: review?(smontagu)
Attached patch patch (obsolete) — Splinter Review
Something like this?
Attachment #308347 - Attachment is obsolete: true
Attachment #308352 - Attachment is obsolete: true
Attachment #308529 - Flags: review?(roc)
Attachment #308529 - Attachment is patch: true
Attachment #308529 - Attachment mime type: application/octet-stream → text/plain
You should share this code with nsPresContext::GetUserPreferences somehow, probably by making a method in nsPresContext that gets shared.
Attachment #308529 - Flags: review?(roc)
Attached patch patch (obsolete) — Splinter Review
Like this, perhaps?
Attachment #308595 - Flags: review?(roc)
Instead of ImageAnimationModePref(), why not just add a method RestoreAnimationMode that sets the mode to the pref? Comment #6 was right :-)
Attached patch patch (obsolete) — Splinter Review
Like this? (I keep trying)
Attachment #308529 - Attachment is obsolete: true
Attachment #308595 - Attachment is obsolete: true
Attachment #308595 - Flags: review?(roc)
Attachment #308757 - Flags: review?(roc)
No, I mean a very small improvement over your last patch; instead of creating ImageAnimationModePref() and calling SetAnimationMode(ImageAnimationModePref()) in a few places, just create a one-line function RestoreAnimationMode() which sets mAnimationMode = mAnimationModePref.
I tried that, but that doesn't work. I need to call SetAnimationMode to reanimate the images, no? Just changing mAnimationMode doesn't magically do that.
OK so have RestoreAnimationMode() call SetAnimationMode(mAnimationModePrefs)
Attached patch patchSplinter Review
Ah, yeah, doh. Sorry about all the patches.
Attachment #308757 - Attachment is obsolete: true
Attachment #308775 - Flags: review?(roc)
Attachment #308757 - Flags: review?(roc)
Assignee: smontagu → martijn.martijn
Comment on attachment 308775 [details] [diff] [review]
patch

This is really safe.
Attachment #308775 - Flags: approval1.9?
Comment on attachment 308775 [details] [diff] [review]
patch

a1.9+=damons
Attachment #308775 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in layout/base/nsDocumentViewer.cpp;
/cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v  <--  nsDocumentViewer.cpp
new revision: 1.580; previous revision: 1.579
done
Checking in layout/base/nsPresContext.h;
/cvsroot/mozilla/layout/base/nsPresContext.h,v  <--  nsPresContext.h
new revision: 3.200; previous revision: 3.199
done
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Sigh, from within a frame, animated images still don't restart animating after printing/print preview, I filed bug 428357 for that.
You need to log in before you can comment on or make changes to this bug.