Last Comment Bug 710990 - Possible duplicate expression in SVGOrientSMILType::Interpolate()
: Possible duplicate expression in SVGOrientSMILType::Interpolate()
Status: RESOLVED FIXED
[pvs-studio]
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description Justin Dolske [:Dolske] 2011-12-14 23:42:26 PST
From http://www.viva64.com/en/a/0078/,
8th section in http://www.viva64.com/external-pictures/txt/mozilla-test.txt

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

nsresult
SVGOrientSMILType::Interpolate(...)
{
  ...
  if (aStartVal.mU.mOrient.mOrientType !=
      nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE ||
      aStartVal.mU.mOrient.mOrientType !=
      nsIDOMSVGMarkerElement::SVG_MARKER_ORIENT_ANGLE) {
    // TODO: it would be nice to be able to handle auto angles too.
    return NS_ERROR_FAILURE;
  }
  ...
Comment 1 Robert Longson 2011-12-15 03:15:05 PST
One of those aStartVal should be an aEndVal
Comment 2 Daniel Holbert [:dholbert] 2011-12-15 11:05:04 PST
This code was added in bug 545042.
Comment 3 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 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 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 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-12-16 08:52:20 PST
hg add anim-marker-orient-02.svg ?
Comment 7 Daniel Holbert [:dholbert] 2011-12-16 09:41:09 PST
It's hg cp'd.  (see last sentence in my last comment :) )
Comment 8 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-12-19 05:03:00 PST
Comment on attachment 582073 [details] [diff] [review]
fix v1 (with reftest)

Oops. :) r=jwatt
Comment 9 Ed Morley [:emorley] 2011-12-20 06:08:58 PST
https://hg.mozilla.org/mozilla-central/rev/4c22853f71ce

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