Closed Bug 683040 Opened 8 years ago Closed 8 years ago

bug 614879 forgot to update the signature on subclass SVGMotionSMILAnimationFunction

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: assertion)

Attachments

(3 files, 3 obsolete files)

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.
Taking; hoping to fix tomorrow morning.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Attached image 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.
Attached patch fix (obsolete) — Splinter Review
This fixes the attached testcase.

Still need to write a reftest or two - I'll post those in a followup patch.
Attachment #557255 - Flags: review?(birtles)
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.
Keywords: assertion
Attached patch fix with reftest (obsolete) — Splinter Review
(here's the fix with a reftest included)
Attachment #557255 - Attachment is obsolete: true
Attachment #557255 - Flags: review?(birtles)
Attachment #557274 - Flags: review?(birtles)
Flags: in-testsuite?
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.)
Attachment #557310 - Flags: review?(birtles)
(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 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)
Attachment #557274 - Flags: review?(birtles) → review+
Attachment #557310 - Flags: review?(birtles) → review+
oops, thanks! :)  I'll fix that before landing.
Attachment #557274 - Attachment is obsolete: true
Attachment #557725 - Flags: review+
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)
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).
Attachment #557725 - Attachment is obsolete: true
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.)
Attachment #558592 - Flags: review?(birtles)
fix v2 passed a reftest/crashtest/mochitest Try run on mac/linux, btw:
  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d33d489b39ec
Comment on attachment 558592 [details] [diff] [review]
fix v2: directly check for |path| attr & <mpath> child

Looks great. Thanks Daniel!
Attachment #558592 - Flags: review?(birtles) → review+
http://hg.mozilla.org/mozilla-central/rev/12507ee12f19
http://hg.mozilla.org/mozilla-central/rev/3ae9b2a742df
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.