Closed Bug 540479 Opened 10 years ago Closed 10 years ago

Add support for SMIL animation of boolean attributes in SVG

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

We should add support for SMIL animation of boolean attributes in SVG.
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #422247 - Flags: review?(dholbert)
Comment on attachment 422247 [details] [diff] [review]
patch to add support for animating boolean attributes

>+nsresult
>+SMILBoolType::ComputeDistance(const nsSMILValue& aFrom,
>+                              const nsSMILValue& aTo,
>+                              double& aDistance) const
>+{
>+  NS_PRECONDITION(aFrom.mType == aTo.mType,"Trying to compare different types");
>+  NS_PRECONDITION(aFrom.mType == this, "Unexpected source type");
>+  aDistance = 0.0;
>+  return NS_ERROR_FAILURE; // there is no concept of distance between bool values
>+}

So in Add() and Interpolate(), both of which also return NS_ERROR_FAILURE, you don't set the outparam.  I think that's the correct xpcom behavior -- we should do the same in ComputeDistance, right?  (i.e. remove the line that sets aDistance to 0.0.)

Clients should be ignoring |aDistance| if we return a failure code, anyway, so setting it to 0 is unnecessary work.

>diff --git a/content/smil/SMILBoolType.h b/content/smil/SMILBoolType.h
>+#ifndef NS_SMILBOOLTYPE_H_
>+#define NS_SMILBOOLTYPE_H_

Shouldn't this lose the NS_ prefix, to match the non-ns-prefixed filename -- e.g. SMILBOOLTYPE_H_ , or perhaps mozilla_SMILBOOLTYPE_H_ ?

I searched MXR for .h files that contain "namespace mozilla", and it looks like there are a multiple conventions for choosing the #defined token -- but of the first few that I saw, none used a NS_ prefix.

>+++ b/content/svg/content/src/nsSVGBoolean.h
+  PRBool GetAnimValue(nsSVGElement *aSVGElement) const
+  {
+  #ifdef MOZ_SMIL
+    aSVGElement->FlushAnimations();
+  #endif
+    return mAnimVal;
+  }
+  
[SNIP]
>+#ifdef MOZ_SMIL
>+  // Returns a new nsISMILAttr object that the caller must delete
>+  nsISMILAttr* ToSMILAttr(nsSVGElement* aSVGElement);
>+#endif // MOZ_SMIL
>+  

In both chunks above, the final line contains extra whitespace.  Nix that whitespace.

r=dholbert with the above addressed.
Attachment #422247 - Flags: review?(dholbert) → review+
One other note:  DidAnimateBool should be called DidAnimateBoolean, for consistency with e.g. DidChangeBoolean, nsSVGBoolean, etc.
Also needs reftests, of course. :)
(And per bug 540478 comment 7, nsSVGFE::DidAnimateBool[ean] should share a helper function, rather than duplicating code)
Thanks for the review. Pushed http://hg.mozilla.org/mozilla-central/rev/b3b2e90f743d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 436276
You need to log in before you can comment on or make changes to this bug.