546785 - nsSVGElement::DidAnimatePreserveAspectRatio function signature doesn't get overridden correctly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

When building mozilla-central, I get this warning:

> content/svg/content/src/nsSVGElement.h:162: warning: ‘virtual void nsSVGElement::DidAnimatePreserveAspectRatio()’ was hidden
> content/svg/content/src/nsSVGFilters.h:224: warning:   by ‘virtual void nsSVGFE::DidAnimatePreserveAspectRatio(PRUint8)’

Both methods were added in bug 541882, and the warning indicates that the subclass method isn't correctly overriding the inherited method -- because the function signatures don't match.

This can be fixed by adding a PRUint8 argument to nsSVGElement::DidAnimatePreserveAspectRatio (which would make it match all the other nsSVGElement::DidAnimateXXX methods).

Comment 1

[Daniel Holbert [:dholbert]]

Alternately, we can remove the PRUint8 arg from nsSVGFE::DidAnimatePreserveAspectRatio -- that might be better, given that there's only one preserveAspectRatio argument, so there's no reason we'd need an attribute index.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> there's only one preserveAspectRatio argument
er, s/argument/attribute/

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix

This patch removes the argument in nsSVGFE's specialization of the method, as described in comment 1.

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 427473 [details] [diff] [review]
fix

Yikes! Well spotted.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Is it possible to mochitest or reftest this?

Comment 6

[Daniel Holbert [:dholbert]]

So, the question is, what should be broken, without this fix...

The only place we call "DidAnimatePreserveAspectRatio()" is inside of nsSVGPreserveAspectRatio::SetAnimValue, here:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGPreserveAspectRatio.cpp#302

So from that call, we'll currently call the nsSVGElement version, missing the nsSVGFE specialization.  This means we miss a call to "DidAnimateAttr" in nsSVGFilters.cpp, and that method would just call InvalidateRenderingObservers on our frame.

So: without this bug's patch, we should be missing some calls to InvalidateRenderingObservers on the frame for our nsSVGFE element.  I'd imagine that issue would be reftestable (though I'm not sure, given that the reftests included with Bug 541882 have apparently been passing).  I'll see if I can make a failing reftest...

Comment 7

[Robert Longson [:longsonr]]

You would need to use MozReftestInvalidate

Comment 8

[Jonathan Watt [:jwatt]]

The only filter effect to use preserveAspectRatio in feImage. This bug was my oversight, so let me whip up a reftest for you.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: reftest - anim-feImage-preserveAspectRatio-01.svg

Comment 10

[Daniel Holbert [:dholbert]]

Thanks jwatt! That reftest (with some MozReftestInvalidate magic added -- thanks for the heads-up Robert) indeed fails pre-patch, and passes post-patch.  (Needed to add the MozReftestInvalidate listener to get it to fail without the patch.)

Comment 11

[Daniel Holbert [:dholbert]]

Landed patch: http://hg.mozilla.org/mozilla-central/rev/34bacacfb0bf
And reftest: http://hg.mozilla.org/mozilla-central/rev/35c956a007a2