I want to switch nsStyleAnimation to use its own union value type rather than nsStyleCoord. The main motivation for this is that I don't want to give nsStyleCoord a nontrivial destructor, and I'd need to do that to support animating complex value types (e.g., shadows, transforms, rects). The separation also makes it clearer which values have which capabilities, although it does result in a drop more duplicated code. (I have the patch mostly written, but not yet tested.)

Comment on attachment 406866 [details] [diff] [review] patch >+++ b/content/smil/nsSMILCSSValueType.cpp [SNIP] > static void >-InvertStyleCoordSign(nsStyleCoord& aStyleCoord) >+InvertStyleCoordSign(nsStyleAnimation::Value& aStyleCoord) The "StyleCoord" in this helper function's title is now misleading -- can we just change it to InvertSign? >+++ b/layout/style/nsStyleAnimation.h I think nsStyleAnimation::Value needs a destructor that just calls FreeValue() (which is a no-op right now, but presumably will clean up after any types that allocate extra space). With that, r=dholbert

addressing dholbert's review comments

Comment on attachment 407015 [details] [diff] [review] patch >+void nsStyleAnimation::Value::SetNormalValue() >+{ >+ FreeValue(); >+ mUnit = eUnit_Normal; >+} One more nit I just noticed -- in the patch, it looks like all of the Value::SetXXXValue() function definitions have the return type (void) and the function name all on the same line. Should the "void" be on its own line, to match local style? (not a big deal -- r=dholbert stands, either way)

(In reply to comment #4) > Should the "void" be on its own line, to match local style? Yes, and I actually already noticed the same thing: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/2c205dc32f42

Comment on attachment 407015 [details] [diff] [review] patch Some of the aStyleCoord arguments should maybe be renamed. I assume there was a reason to not make nsStyleAnimation::Value just inherit from nsStyleCoord and have a less trivial ctor?

(In reply to comment #6) > I assume there was a reason to not make nsStyleAnimation::Value just inherit > from nsStyleCoord and have a less trivial ctor? Because that would require making either all the Set* methods on nsStyleCoord or something that they call virtual.