683040 - bug 614879 forgot to update the signature on subclass SVGMotionSMILAnimationFunction

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

jcranmer told me on IRC that 
>88   NS_OVERRIDE virtual PRBool TreatSingleValueAsStatic() const;
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGMotionSMILAnimationFunction.h#88

is causing problems in static-analysis builds since the overridden method has been removed. 

This function needs to be removed/replaced.

Comment 1

[Daniel Holbert [:dholbert]]

Taking; hoping to fix tomorrow morning.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase

Here's a testcase demonstrating what's broken here.

Basically, we have special-case handling for animation elements with the "to" attribute & no from/values attributes.

However, for <animateMotion>, the |path| attribute and <mpath> sub-element are both supposed to override "to".  Currently, if there's "to" and a path/mpath-specified animateMotion (the orange boxes in the attached testcase), we behave oddly -- snapping between endpoints rather than animating smoothly.

We need to override "IsToAnimation" (added in bug 614879) to fix this.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix

This fixes the attached testcase.

Still need to write a reftest or two - I'll post those in a followup patch.

Comment 4

[Daniel Holbert [:dholbert]]

FWIW, the attached testcase & upcoming reftest also trigger this assertion in unpatched builds:
> ###!!! ASSERTION: Unexpected number of values: 'Error', file ../../../mozilla/content/smil/nsSMILAnimationFunction.cpp, line 396

I've verified that the attached patch fixes this assertion-failure.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix with reftest

(here's the fix with a reftest included)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: animateMotion-tweaked copies of bug 614879's reftests

Just for thoroughness, here are 2 more reftests -- copies of bug 614879's first 2 reftests, but tweaked to be for <animateMotion> instead of <animate>. (and tweaked to match lime.svg, since that's the standard reference case in the smil/motion reftests dir)

(These bonus reftests pass already, regardless of the fix on this bug here, but they fail in builds before bug 614879 landed.)

Comment 7

[Daniel Holbert [:dholbert]]

(note that the bonus reftests patch I just attached uses "copy from... copy to..." w/ diffs on the copies, which bugzilla's diff viewer interprets as modifying the original files.  Don't let that alarm you. :))

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 557274 [details] [diff] [review]
fix with reftest

Looks good. (Also, since I know you care about this sort of thing, jst's review tool says there is some trailing whitespace: http://beaufour.dk/jst-review/?patch=557274)

Comment 9

[Daniel Holbert [:dholbert]]

oops, thanks! :)  I'll fix that before landing.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix with reftest (whitespace fixed, noted r=birtles)

Comment 11

[Daniel Holbert [:dholbert]]

trailing-whitespace-free now. :)  http://beaufour.dk/jst-review/?patch=557725

(thanks for that link, BTW - I didn't know it was so easy to generate auto-jst-review URLs)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v2: directly check for |path| attr & <mpath> child

As discussed last week on IRC, the previous fix failed the mochitest "test_smilAnimateMotionInvalidValues.xhtml" -- it wasn't changing the CTM immediately after a "pure to" animateMotion animation was applied.

This was actually due to a bug in the previous patch. IsToAnimation was relying on mPathSourceType, which doesn't get set to a useful value until we reach SVGMotionSMILAnimationFunction::GetValues(), and we depend on IsToAnimation() working before then.

(In the specific case of the mochitest failure, WillReplace was getting the wrong answer from IsToAnimation(), and so WillReplace returned the wrong answer ("true"), making us think we'd replace the underlying value.  This made us skip the base-value lookup -- but then when we try to use the base value later, we bail out and don't animate.

This was only a problem in the very first sample (since after that mPathSourceType is set) -- but luckily this mochitest depends on the very first sample being correct, so it caught the bug. :)

Anyway, the fix is to directly check for the |path| attribute and the <mpath> sub-element (those being the additional features that animateMotion brings along which can override the "to" attribute).

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 558592 [details] [diff] [review]
fix v2: directly check for |path| attr & <mpath> child

(Note: The only differences in this new patch vs. the old one are in SVGMotionSMILAnimationFunction.h/cpp.  The tests are unchanged.)

Comment 14

[Daniel Holbert [:dholbert]]

fix v2 passed a reftest/crashtest/mochitest Try run on mac/linux, btw:
  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d33d489b39ec

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 558592 [details] [diff] [review]
fix v2: directly check for |path| attr & <mpath> child

Looks great. Thanks Daniel!

Comment 16

[Daniel Holbert [:dholbert]]

Thanks!
 http://hg.mozilla.org/integration/mozilla-inbound/rev/12507ee12f19
 http://hg.mozilla.org/integration/mozilla-inbound/rev/3ae9b2a742df

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/12507ee12f19
http://hg.mozilla.org/mozilla-central/rev/3ae9b2a742df