switch nsStyleAnimation to its own union value type rather than nsStyleCoord

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.)
(Assignee)

Comment 1

9 years ago
Created attachment 406866 [details] [diff] [review]
patch
Attachment #406866 - Flags: review?(dholbert)
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 3

9 years ago
Created attachment 407015 [details] [diff] [review]
patch

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)
(Assignee)

Comment 5

9 years ago
(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
(Assignee)

Updated

9 years ago
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?
(Assignee)

Comment 7

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2072cf8f65b4
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Depends on: 529356
(Assignee)

Updated

8 years ago
Blocks: 537142
You need to log in before you can comment on or make changes to this bug.