Add support for SMIL animation of enumeration attributes in SVG

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

We should add support for SMIL animation of enumeration attributes in SVG.
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #422245 - Flags: review?(dholbert)
Attachment #422246 - Attachment is patch: true
Attachment #422246 - Attachment mime type: application/octet-stream → text/plain
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?
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 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?
Attachment #422245 - Flags: review?(dholbert) → review+
(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 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 :))
Attachment #422246 - Flags: review?(dholbert) → review+
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. :-)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
s/more than one tests/more tests than can be encoded in one decimal digit/
Blocks: 436276
You need to log in before you can comment on or make changes to this bug.