540479 - Add support for SMIL animation of boolean attributes in SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

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

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch to add support for animating boolean attributes

Comment 2

[Daniel Holbert [: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.

Comment 3

[Daniel Holbert [:dholbert]]

One other note:  DidAnimateBool should be called DidAnimateBoolean, for consistency with e.g. DidChangeBoolean, nsSVGBoolean, etc.

Comment 4

[Daniel Holbert [:dholbert]]

Also needs reftests, of course. :)

Comment 5

[Daniel Holbert [:dholbert]]

(And per bug 540478 comment 7, nsSVGFE::DidAnimateBool[ean] should share a helper function, rather than duplicating code)

Comment 6

[Jonathan Watt [:jwatt]]

Thanks for the review. Pushed http://hg.mozilla.org/mozilla-central/rev/b3b2e90f743d