Closed Bug 522852 Opened 12 years ago Closed 12 years ago

switch nsStyleAnimation to its own union value type rather than nsStyleCoord

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.)
Attached patch patch (obsolete) — Splinter Review
Attachment #406866 - Flags: review?(dholbert)
Attachment #406866 - Flags: review?(bzbarsky)
Attachment #406866 - Flags: review?(dholbert) → review+
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
Attached patch patchSplinter Review
addressing dholbert's review comments
Attachment #406866 - Attachment is obsolete: true
Attachment #407015 - Flags: review?(bzbarsky)
Attachment #406866 - Flags: review?(bzbarsky)
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
Blocks: 523196
Attachment #407015 - Flags: review?(bzbarsky) → review+
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.
http://hg.mozilla.org/mozilla-central/rev/2072cf8f65b4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Depends on: 529356
You need to log in before you can comment on or make changes to this bug.