710990 - Possible duplicate expression in SVGOrientSMILType::Interpolate()

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Justin Dolske [:Dolske]]

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 [:longsonr]]

One of those aStartVal should be an aEndVal

Comment 2

[Daniel Holbert [:dholbert]]

This code was added in bug 545042.

Comment 3

[Daniel Holbert [:dholbert]]

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]]

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]]

Attachment: 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]]

hg add anim-marker-orient-02.svg ?

Comment 7

[Daniel Holbert [:dholbert]]

It's hg cp'd.  (see last sentence in my last comment :) )

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 582073 [details] [diff] [review]
fix v1 (with reftest)

Oops. :) r=jwatt

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c22853f71ce