Closed Bug 545550 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch wip (just for nsSVGLength2) (obsolete) — Splinter Review
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.
Attached patch fix (obsolete) — Splinter Review
This patch applies the same fix to the boolean, enum, number, and preserveAspectRatio types. (in addition to length, which was in the previous patch)
Attachment #426392 - Attachment is obsolete: true
Attachment #426398 - Flags: review?
Attachment #426398 - Flags: review? → review?(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. :-)
Attachment #426398 - Flags: review?(jwatt) → review+
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.
I think the only time it's *not* required for a baseval change is under SetAttr.
Attached patch fix v2 [r=jwatt]Splinter Review
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...
Attachment #426398 - Attachment is obsolete: true
Attachment #426546 - Flags: review+
Landed: http://hg.mozilla.org/mozilla-central/rev/165e0bfa8352
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 571969
Depends on: 593182
Depends on: 601934
You need to log in before you can comment on or make changes to this bug.