Closed Bug 611797 Opened 9 years ago Closed 9 years ago

Move GetAnimationMode/SetAnimationMode from RasterImage/VectorImage to Image


(Core :: ImageLib, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)




(2 files)

Just noticed that RasterImage & VectorImage each have their own implementations for GetAnimationMode/SetAnimationMode, when they really should be sharing an implementation.

Filing this bug on promoting those methods to the Image superclass, (plus related minor cleanup).
Summary: Move GetAnimationMode/SetAnimationMode from RasterImage to Image → Move GetAnimationMode/SetAnimationMode from RasterImage/VectorImage to Image
So, GetAnimationMode/SetAnimationMode are defined in imgIContainer.

Annoyingly, if you use the NS_DECL_IMGICONTAINER macro in a class, it forces you to have definitions for _every_ imgIContainer method there.  But this bug wants to have 2 methods defined in a superclass (Image) and the rest defined in its subclasses (Raster/VectorImage), which NS_DECL_IMGICONTAINER doesn't let you do.

dbaron says the standard workaround for this is just to replace NS_DECL_IMGICONTAINER with the equivalent lines from the auto-generated .h file, and then shift lines between classes as necessary.

This patch does the first part -- it just expands NS_DECL_IMGICONTAINER.
Attachment #490197 - Flags: review?
Attachment #490197 - Flags: review? → review?(joe)
This patch:
 - shifts the mAnimationMode member var to Image superclass.
 - shifts the SetAnimationMode() & GetAnimationMode() declaration/definition to Image superclass. (using the impl's from RasterImage)
 - Shift the image-mode check from RasterImage's "ShouldAnimate" method to the Image superclass
 - Adds a custom ShouldAnimate() impl in VectorImage, to simplify StartAnimation's checks
 - Add some sanity-check assertions to VectorImage::StartAnimation/StopAnimation, which should hold as long as the only caller of those methods is Image::EvaluateAnimation
Attachment #490200 - Flags: review?(joe)
Note that the patches here make SVG images match raster images in falling prey to Bug 601723 -- stopping animation after a reload.  (Up to now, they hadn't,  probably because VectorImage's StartAnimation/StopAnimation impls weren't quite right.)

I think this change is a Good Thing -- it's just an indication of RasterImage & VectorImage sharing more code & behaving more consistently, and since Bug 601723 is blocking-final+ anyway, it'll be fixed (for both, presumably) before we release. (Once it's fixed, we'll just need to verify that it's indeed fixed both for Raster images and for SVG images.)
Comment on attachment 490197 [details] [diff] [review]

Attachment #490197 - Flags: review?(joe) → review+
Comment on attachment 490200 [details] [diff] [review]
Patch 2: Shift code to Image superclass

In VectorImage::StartAnimation and VectorImage::StopAnimation, you change some conditionals that just return NS_OK to NS_ABORT_IF_FALSEs. How did you convince yourself that those conditions never happened?
Attachment #490200 - Flags: review?(joe) → review+
We can be sure that those conditions will never happen because now there's only one client of VectorImage::StartAnimation & StopAnimation, which is EvaluateAnimations, and it's got strict & simple conditions about when it calls them.

For the checks removed from StartAnimation:
  EvaluateAnimation only calls StartAnimation if "ShouldAnimate()" is true.  So I've just shifted those checks from StartAnimation into ShouldAnimate, and if the checks fail, they just keep us from calling StartAnimation in the first place.

For the checks removed from StopAnmation:
  EvaluateAnimation only calls StopAnimation if we're currently animating -- if ShouldAnimate() returned true for this image at some point in the past.  Now, if the conditions that I removed from StopAnimation() were actually met, then ShouldAnimate() could never have returned true, so we wouldn't ever get a call to StopAnimation().
Sounds good.
Comment on attachment 490197 [details] [diff] [review]

Requesting approval to land both patches here.  (goal: share more code between our two Image subclasses, and simplify assumptions made in the VectorImage class to be more consistent with RasterImage.)
Attachment #490197 - Flags: approval2.0?
Attachment #490200 - Flags: approval2.0?
Attachment #490197 - Flags: approval2.0? → approval2.0+
Attachment #490200 - Flags: approval2.0? → approval2.0+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Depends on: 620810
You need to log in before you can comment on or make changes to this bug.