545550 - SVG/SMIL: Setters for SVG attr base value shouldn't directly modify animated value, when animations are active

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Bug 533291 has us avoid recomputing an attribute's animated value, when the animation functions targeting that attribute haven't changed since the previous sample.

One particularly interesting case is when only the **base value** has changed since the previous sample, and we can tell that the base value will be ignored (e.g. because it's got a "from/to" or "from/by" animation clobbering it in the animation sandwich).  In that case, we shouldn't need to recompute the animated value, because we know it won't change.

However, if I optimize that recomputation away, we currently start failing tests, because most setters for SVG attributes' base values will currently *also* set the animated value -- e.g.:
460 void
461 nsSVGLength2::SetBaseValue(float aValue, nsSVGElement *aSVGElement)
462 {
463   mAnimVal = mBaseVal = 
464     aValue * GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType);

In these setters, I think we want to leave mAnimVal alone. (except when it's not being animated, which we can tell from mIsAnimated)

Filing this bug on making that change.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: wip (just for nsSVGLength2)

Here's an initial fix, just for nsSVGLength2.  I still need to do this for the other SVG attribute-types that we support for animation.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix

This patch applies the same fix to the boolean, enum, number, and preserveAspectRatio types. (in addition to length, which was in the previous patch)

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 426398 [details] [diff] [review]
fix

>+  // XXX shouldn't we be calling DidChangeBoolean here?

No. I wondered about that myself previously and ended up going through the code to check, so it's probably useful to add a comment something like:

  // We're only called by nsSVGElement::ParseAttribute under
  // nsGenericElement::SetAttr, which takes care of notifying.

You can also replace the DidChangeLength call in nsSVGLength2::SetBaseValueString with that comment. Similar thing for nsSVGPreserveAspectRatio and any other classes with SetBaseValueString you can find. :-)

Comment 4

[Robert Longson [:longsonr]]

FWIW my understanding was that DidChange calls were required only if we were going to do anything unusual i.e. when it needed to be overridden i.e. in Marker, Use and Script.

Comment 5

[Jonathan Watt [:jwatt]]

I think the only time it's *not* required for a baseval change is under SetAttr.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2 [r=jwatt]

Thanks jwatt! I replaced all those XXX comments with a slightly extended version of your suggested text.

As you suggested, I also replaced actual calls of DidChange* inside of SetBaseValueString, everywhere I could find 'em -- in nsSVGPreserveAspectRatio, nsSVGLength2, and nsSVGAngle. (might cause minor bitrot in your <angle> patch in bug 545042, but hopefully not much -- it's just a few-line change in that one method)

Tree permitting, I'll land this shortly...

Comment 7

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/165e0bfa8352