522852 - switch nsStyleAnimation to its own union value type rather than nsStyleCoord

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

addressing dholbert's review comments

Comment 4

[Daniel Holbert [:dholbert]]

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)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/2072cf8f65b4