540478 - Add support for SMIL animation of enumeration attributes in SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We should add support for SMIL animation of enumeration attributes in SVG.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch to make GetAnimVal take its element as an argument

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch to add support for animating enum attributes

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 422246 [details] [diff] [review]
patch to add support for animating enum attributes

>diff --git a/content/smil/Makefile.in b/content/smil/Makefile.in
> 	nsSMILValue.cpp \
>+	SMILEnumType.cpp \

Why no "ns" prefix here? (nsSMILEnumType)  Same in bug 540479.  Are we moving away from that?

Comment 4

[Jonathan Watt [:jwatt]]

Yes, there's a move across the whole moz codebase to using the 'mozilla' namespace to replace 'ns' now that namespaces are officially allowed in moz code.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 422246 [details] [diff] [review]
patch to add support for animating enum attributes

>+        mBaseVal = mAnimVal = mapping->mVal;
>+        #ifdef MOZ_SMIL
>+        if (mIsAnimated) {
>+          aSVGElement->AnimationNeedsResample();
>+        }
>+        #endif
>+      }

I think the #ifdef and #endif should be flush left. I think this occurs in a couple of places.

More importantly are you planning to do auto angled markers as part of an angle animation patch?

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to comment #5)
> I think the #ifdef and #endif should be flush left. I think this occurs in a
> couple of places.

I can do that on checkin. (Personally I find it more readable the way I have it, but I don't care all that much.)

> More importantly are you planning to do auto angled markers as part of an angle
> animation patch?

Yes, I have a separate patch for that. I'm still fiddling with tests for the other patches that I have. I'll upload the others soon.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 422246 [details] [diff] [review]
patch to add support for animating enum attributes

Similar to bug 540479 comment 2:
  - SMILEnumType::ComputeDistance probably shouldn't bother modifying its outparam
  - SMILEnumType.h possibly shouldn't have the "NS_" prefix on its #define (and should use mozilla_ instead)?
  - Nix whitespace-on-blank-line after GetAnimValue definition (in nsSVGEnum.h)

And, +1 to Robert's note about #ifdef indentation in nsSVGEnum::SetBaseValueString & SetBaseValue.

>+nsSVGEnum::SMILEnum::SetAnimValue(const nsSMILValue& aValue)
>+{
>+  NS_ASSERTION(aValue.mType == &SMILEnumType::sSingleton,
>+               "Unexpected type to assign animated value");
>+  if (aValue.mType == &SMILEnumType::sSingleton) {
>+    mVal->SetAnimValue(PRUint16(aValue.mU.mUint), mSVGElement);

So we're converting a 64-bit unsigned integer (aValue.mU.mUint) into a 16-bit unsigned integer.  We should assert that we're not overflowing the 16-bit value there, because we should never be dealing with enumerated values that large (and if we end up with one, something's broken).

So anyway, just before the conversion, we should add something like:
 NS_ABORT_IF_FALSE(aValue.mU.mUint <= USHRT_MAX,
                   "out of bounds enumerated value");

>+void
>+nsSVGFE::DidAnimateEnum(PRUint8 aAttrEnum)
>+{
>+  // nsSVGLeafFrame doesn't implement AttributeChanged.
>+  nsIFrame* frame = GetPrimaryFrame();
>+  if (frame) {
>+    nsSVGEffects::InvalidateRenderingObservers(frame);
>+  }
>+}
>+

So this gives us three functions with the exact same body.  Bug 540479 adds a fourth, "DidAnimateBool".  Can we avoid this duplication by refactoring this method body into a helper function that each of these methods can call?

The same applies to nsSVGFilterElement::DidAnimateEnum, though that one's not as bad since it only has two of these duplicate-functions after this patch.  Still, the same principle applies -- we should probably be using a helper method there, too. (It can be inline, if you want to avoid the overhead of making a function call.)

>diff --git a/layout/reftests/svg/smil/anim-filter-filterUnits-01.svg b/layout/reftests/svg/smil/anim-filter-filterUnits-01.svg

Both reftests have some very long lines, making them a bit hard to read. (longest line is 112 chars in both cases)  Could you prettify the line-wrapping, ideally to < 80 chars?

Also -- nit :) -- should these reftests be named "XXX-1.svg", rather than "XXX-01.svg"? I think the former is more common in mozilla tests, and in this case, it looks like there's only one version of each test, which makes the two-digit numbering seem excessive.  (Maybe we'll branch off a few subtests later, but probably not 10 more...?)

r=dholbert with the above addressed (or rebutted :))

Comment 8

[Jonathan Watt [:jwatt]]

Pushed:

http://hg.mozilla.org/mozilla-central/rev/df15d308c851
http://hg.mozilla.org/mozilla-central/rev/03641989306e

(In reply to comment #7)
> Also -- nit :) -- should these reftests be named "XXX-1.svg", rather than
> "XXX-01.svg"? I think the former is more common in mozilla tests, and in this
> case, it looks like there's only one version of each test, which makes the
> two-digit numbering seem excessive.  (Maybe we'll branch off a few subtests
> later, but probably not 10 more...?)

The -xx convention is standard for the SVG reftests. For many there will be more than one tests in future (or that's my intention), and it seems better to me to start out with consistency rather than changing convention for some of them as we roll over the limit. It's only one extra character after all. :-)

Comment 9

[Jonathan Watt [:jwatt]]

s/more than one tests/more tests than can be encoded in one decimal digit/