Last Comment Bug 710990 - Possible duplicate expression in SVGOrientSMILType::Interpolate()
: Possible duplicate expression in SVGOrientSMILType::Interpolate()
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla11
Assigned To: Daniel Holbert [:dholbert]
: Jet Villegas (:jet)
Depends on:
Blocks: 710966 545042
  Show dependency treegraph
Reported: 2011-12-14 23:42 PST by Justin Dolske [:Dolske]
Modified: 2012-01-30 08:52 PST (History)
4 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix v1 (with reftest) (4.42 KB, patch)
2011-12-15 13:07 PST, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review

Description User image Justin Dolske [:Dolske] 2011-12-14 23:42:26 PST
8th section in

V501 There are identical sub-expressions to the left and to the right of the '||' operator.
svgorientsmiltype.cpp 161

  if (aStartVal.mU.mOrient.mOrientType !=
      aStartVal.mU.mOrient.mOrientType !=
    // TODO: it would be nice to be able to handle auto angles too.
    return NS_ERROR_FAILURE;
Comment 1 User image Robert Longson 2011-12-15 03:15:05 PST
One of those aStartVal should be an aEndVal
Comment 2 User image Daniel Holbert [:dholbert] 2011-12-15 11:05:04 PST
This code was added in bug 545042.
Comment 3 User image Daniel Holbert [:dholbert] 2011-12-15 11:18:30 PST
Right now if we try to interpolate for an animation with e.g.
  from="180deg" to="auto"
we end up treating "auto" as if it were "0deg", at least in my debug build, because that's the (bogus) angle-value associated with our "auto" value in the interpolation code. It may be that in optimized builds we end up with a nonzero value there & get random results.

(However, once the animation completes, if it's got fill='freeze', we'll skip the interpolation call and just directly set the correct "auto" value.)
Comment 4 User image Daniel Holbert [:dholbert] 2011-12-15 11:19:57 PST
I'll just take this; it affects behavior, so best to fix it quickly, and I've already got a testcase in mind based on the previous comment.
Comment 5 User image Daniel Holbert [:dholbert] 2011-12-15 13:07:18 PST
Created attachment 582073 [details] [diff] [review]
fix v1 (with reftest)

This includes a reftest for animating the "orient" attribute to the value "auto".

The reftest checks two things:
 (a) completed frozen animations to "auto" should end up at "auto"
 (b) in-progress animations to "auto" should fall back to discrete-mode (still be at the initial value for at least the first half of the animation), since interpolation fails.

We succeed at (a) already, but current trunk fails at (b) as described in comment 3.  I verified that the reftest as a whole fails pre-patch and passes post-patch.

(I used "hg cp" to create the reftest, so don't be tricked by bugzilla's diff viewer making it look like I'm modifying an existing file. :))
Comment 6 User image Jonathan Watt [:jwatt] 2011-12-16 08:52:20 PST
hg add anim-marker-orient-02.svg ?
Comment 7 User image Daniel Holbert [:dholbert] 2011-12-16 09:41:09 PST
It's hg cp'd.  (see last sentence in my last comment :) )
Comment 8 User image Jonathan Watt [:jwatt] 2011-12-19 05:03:00 PST
Comment on attachment 582073 [details] [diff] [review]
fix v1 (with reftest)

Oops. :) r=jwatt
Comment 9 User image Ed Morley [:emorley] 2011-12-20 06:08:58 PST

Note You need to log in before you can comment on or make changes to this bug.