611797 - Move GetAnimationMode/SetAnimationMode from RasterImage/VectorImage to Image

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

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).

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Patch 1: expand NS_DECL_IMGICONTAINER

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Patch 2: Shift code to Image superclass

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

Comment 3

[Daniel Holbert [:dholbert]]

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 4

[Joe Drew (not getting mail)]

Comment on attachment 490197 [details] [diff] [review]
Patch 1: expand NS_DECL_IMGICONTAINER

ugggh

Comment 5

[Joe Drew (not getting mail)]

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?

Comment 6

[Daniel Holbert [:dholbert]]

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().

Comment 7

[Joe Drew (not getting mail)]

Sounds good.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 490197 [details] [diff] [review]
Patch 1: expand NS_DECL_IMGICONTAINER

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.)

Comment 9

[Daniel Holbert [:dholbert]]

Landed: 
  http://hg.mozilla.org/mozilla-central/rev/a65974f1daad
  http://hg.mozilla.org/mozilla-central/rev/453184e9da05