504652 - create nsStyleAnimation class for common interpolation and value conversion code

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

I've been thinking about what we need for the code that's shared between
CSS transitions and SMIL animation.  For CSS transitions, the inputs and
outputs are always computed values (what we store in nsStyleStruct).
For SMIL animation of CSS, the inputs can be either specified or
computed values, and the output is a specified value.

My current thought is that it's probably easier to animate between
computed values, since there is less type incompatibility to deal with.

So I'd propose first that we extend nsStyleCoord to be able to hold the
computed values of all properties that we want to animate.  We can do
this gradually as we animate more types.

Then I propose that we have a class call nsStyleAnimation that has five
main public methods (I suspect they'll all need some additional
parameters to the ones I give).  The first two are needed for both
transitions and SMIL, the third for transitions only, and the the last
two are needed only for SMIL.  I think they should probably be written
in four (or more) separate patches.  (For the first two, we might want
to write them one value type at a time.)

 (1) Interpolate(const nsStyleCoord &aStartValue,
                 const nsStyleCoord &aEndValue,
                 double aPortion,
                 nsStyleCoord &aResult);
     which returns the interpolated result when aPortion-of-the-way
     through an animation from aStartValue to aEndValue, if it's
     possible to animate between them (i.e., they are compatible types).
     (The caller will likely have already used nsSMILKeySpline to
     compute aPortion.)

 (1a) It seems SMIL paced animations also need a Distance method here
      that's similar to interpolate, but instead computes a distance
      between the two values.

 (2) ExtractComputedValue(const nsStyleContext *aStyleContext,
                          nsCSSProperty aProperty,
                          nsStyleCoord& aComputedValue);
 (3) StoreComputedValue(void *aStyleStruct,
                        nsCSSProperty aProperty,
                        const nsStyleCoord& aComputedValue);

     These two would share almost all of the infrastructure in a common
     helper method.  That common helper method would need to be based
     off some pretty significant additions to nsCSSPropList.h (new
     fields) describing value types and locations within style structs.


 (4) ComputeValue(nsCSSProperty aProperty,
                  const nsStyleContext *aStyleContext,
                  const ??? aSpecifiedValue, 
                  nsStyleCoord& aComputedValue);
     This turns a specified value for a property * element into a
     computed value without actually changing that element's style data.
     It uses nsCSSExpandedDataBlock (which might actually just be its
     input) to set up a simple nsIStyleRule implementation that stores
     that single value (at highest priority), creates a rule node for
     that rule below the style context's rule node, makes a style
     context for that rule node, computes the style, and then calls
     ExtractComputedValue (above).  Or something like that.  (It could
     be optimized further later.)  The result of this function would
     typically be input to (1).

 (5) UncomputeValue(nsCSSProperty aProperty,
                    const nsStyleCoord &aComputedValue,
                    ??? aSpecifiedValue);
     This does the reverse of (4); it turns a computed value (generally
     output from (1)) into a specified one.  Hopefully this can be done
     largely using the same type data used for (2) and (3) with a switch
     over the value types.

     Or if this is hard (see below), we could cheat and use the
     nsDOMComputedStyle code to convert to a string (*without* giving it
     a frame, so that it doesn't move things past computed values) and
     then use the CSS parser to parse that string.  It might make more
     sense to start with this approach.  But we'd definitely want to
     poke a new hole into nsComputedDOMStyle rather than calling the
     normal codepath that deals with frames.


     A relatively easy prerequisite for (5) (the non-easy version),
     which should be done in a separate patch, is that we need to fix
     what I think was a bad design decision:  we need to put
     eCSSKeyword_auto, eCSSKeyword_none, etc., in keyword tables ***in
     the case where*** nsRuleNode maps eCSSUnit_Auto to an enumerated
     type (e.g., for the cursor property).  This will add a single entry
     to an array and let us eliminate a few lines of code in nsRuleNode
     and nsComputedDOMStyle for each of these properties.  However, we
     don't want to do this for properties where eCSSKeyword_auto, etc.,
     compute to nsStyleCoord's auto value (e.g., 'width').  Examples of
     properties to fix are cursor: auto, outline-style: none,
     border-*-style: none, column-rule-style: none, 

     We would probably also want to make some other prerequisite changes
     in separate patches to improve consistency of how we store
     properties in style structs (e.g., making mStretchStack an integer
     rather than boolean).

     The question as to how hard this is is how many such prerequisite
     patches would be needed to make the main patch not-too-big.  I guess
     my inclination right now is actually that we should just try the
     easy version.



Thoughts on this plan?

Comment 1

[Boris Zbarsky [:bzbarsky]]

The plan sounds generally fine; I assume by "easy version" you mean "use nsComputedDOMStyle"?  I'm all in favor of doing that and seeing how it performs...

Comment 2

[Daniel Holbert [:dholbert]]

This all sounds good to me.

> Or if this is hard (see below), we could cheat

Just to be clear, what do you mean by "cheat" here? (regarding the "easy" implementation for #5)  Is it "cheating" in the sense that it would sometimes produce the wrong results?  Or that it requires poking a hole to access some private methods?  Or that it's potentially less efficient than the alternative? (I'm mostly worried about the first possibility -- incorrect results.  If that's not an issue, then I think I'm all for "cheating" to start out with, and then improving on that if necessary. :))

Comment 3

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

(In reply to comment #1)
> The plan sounds generally fine; I assume by "easy version" you mean "use
> nsComputedDOMStyle"?  I'm all in favor of doing that and seeing how it
> performs...

Yes.

(In reply to comment #2)
> Just to be clear, what do you mean by "cheat" here? (regarding the "easy"
> implementation for #5)  Is it "cheating" in the sense that it would sometimes
> produce the wrong results?  Or that it requires poking a hole to access some
> private methods?  Or that it's potentially less efficient than the alternative?
> (I'm mostly worried about the first possibility -- incorrect results.  If
> that's not an issue, then I think I'm all for "cheating" to start out with, and
> then improving on that if necessary. :))

I think it's got two potential downsides:
 * it's slower
 * it might not produce the right results in all cases, though we can probably fix those cases

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This sounds great.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
>  (1) Interpolate(const nsStyleCoord &aStartValue,
[snip]
>  (1a) It seems SMIL paced animations also need a Distance method here

SMIL needs one more method here -- an "Add" method.  This is for "by" animations, in which we aren't explicitly given the end value -- we're just given a value to progressively add to our base value.

See the declaration of this function's equivalent in nsISMILType.h:
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsISMILType.h

This would look something something like:
 (1b) Add(nsStyleCoord& aDest,
          const nsStyleCoord& aValueToAdd,
          PRUint32 aCount);

(The "aCount" parameter is there because the animation may additively repeat, adding successively more copies of the "by" value to our base value.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: patch 1

Here's a WIP patch for this.

Of the methods mentioned in comment 1 and comment 5, this patch has implementations for:
 - Add, ComputeDistance, Interpolate, ComputeValue, ExtractComputedValue
It's missing implementations for:
 - StoreComputedValue, UncomputeValue

I've reworked my smil_css patch from bug 474049 to build on top of this code now, and I'll be posting an updated patch there soon.

Note this comment in ExtractComputedValue:
> +  // XXXdholbert This is a simple implementation that only supports "font-size"
> +  // and "stroke-width for now, so that we can test the rest of the
> +  // functionality. A full implementation will require modifications to
> +  // nsCSSPropList.h, as described in bug 504652 comment 0.

Since this function is currently limited as described there, this patch only works with font-size and stroke-width, so far.

Comment 7

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

Comment on attachment 395708 [details] [diff] [review]
patch 1

>+/**
>+ * Helper Class: StyleRuleImpl
>+ * This class is a simple nsIStyleRule implementation, which simply wraps a
>+ * nsCSSDeclaration.
>+ */

I don't think you need this.  Where you create it, you should be able to just do

nsCOMPtr<nsICSSStyleRule> styleRule;
rv = NS_NewCSSStyleRule(getter_AddRefs(styleRule), nsnull, aDeclaration);
NS_ENSURE_SUCCESS(rv, rv);

>+PRBool
>+nsStyleAnimation::ExtractComputedValue(nsCSSProperty aProperty,
>+                                       nsStyleContext* aStyleContext,
>+                                       nsStyleCoord& aComputedValue)
>+{
>+  // XXXdholbert This is a simple implementation that only supports "font-size"
>+  // and "stroke-width for now, so that we can test the rest of the
>+  // functionality. A full implementation will require modifications to
>+  // nsCSSPropList.h, as described in bug 504652 comment 0.

Maybe I could take a pass at writing this part?  Would that make sense?

Comment 8

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

(In reply to comment #7)
> Maybe I could take a pass at writing this part?  Would that make sense?

I'm working on doing this; I have code that's compiling, and I've restructured my transitions patch to work on top of it, but it's not running yet.

Comment 9

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

I think I've gotten transitions behaving pretty much as they were before I switched to nsStyleAnimation; the patch queue I used to do that is:

http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1f2f15e75aa3/css-prop-stylestruct
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1f2f15e75aa3/more-cssproplist-fields
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1f2f15e75aa3/style-animation-style-coord
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1f2f15e75aa3/style-animation-nscoord
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1f2f15e75aa3/style-animation-color
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1f2f15e75aa3/style-animation-paint-server

Comment 10

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

And note that my current versions of those patches can be obtained by replacing the revision ID in those URLs with "tip"... as long as they're still in my patch queue under those names.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> (In reply to comment #7)
> > Maybe I could take a pass at writing this part?  Would that make sense?
> 
> I'm working on doing this; I have code that's compiling, and I've restructured
> my transitions patch to work on top of it, but it's not running yet.

Yes --- looks great, thanks!

Comment 12

[Daniel Holbert [:dholbert]]

I've updated my SMIL + CSS code so that it behaves as it did before I switched to nsStyleAnimation, **just** for the units that we support so far with my base styleanimation patch & dbaron's patches from comment 9. (Those supported units are: lengths, percentages, colors, and paints-that-are-actually-just-colors).  These changes are all in my patch queue -- here's the series file, showing all the patches that need to be qpushed for this support (with the qguard "styleanimation"):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/tip/series

The patches in my patch queue update the SMIL reftest.list files to indicate known failures for units that don't work yet. (e.g. tests for opacity & the "font" shorthand, and tests that mix px and percent values for stroke-width)

Comment 13

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

Attachment: patch 2: add stylestruct_ parameter to CSS_PROP macro

This actually turned out not to be useful yet, but I think it's good cleanup and it could be more useful later (since once we have all or almost all of the properties done, we can list only the second parameter of the offsetof() in nsCSSPropList.h).  And all the rest of my patches are on top of it...

Comment 14

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

Attachment: patch 3: Add fields to nsCSSPropList.h for style struct offset and animation type

Note that I rewrapped some of the (already, but now worse) overly long lines in patch 4, but that's whitespace-only changes so I won't even bother requesting review on it.

Comment 15

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

Attachment: patch 4: fix overly long lines from patches 2 and 3

Comment 16

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

Attachment: patch 5: add support for animation of nsStyleCoord and nsStyleSides to nsStyleAnimation

This adds support for the first few real nsStyleAnimType values to nsStyleAnimation.  It replaces the stub for ExtractComputedValue and adds StoreComputedValue.

Comment 17

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

Attachment: patch 6: add support for animation of nscoord values to nsStyleAnimation

Comment 18

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

Attachment: patch 7: add support for color values to nsStyleCoord

Comment 19

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

Attachment: patch 8: add support for animation of nscolor values and eStyleUnit_Color coords to nsStyleAnimation

Comment 20

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

Attachment: patch 9: add support for animation of colors in paint server values to nsStyleAnimation

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 396486 [details] [diff] [review]
patch 5: add support for animation of nsStyleCoord and nsStyleSides to nsStyleAnimation

>diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp
> PRBool
> nsStyleAnimation::ExtractComputedValue(nsCSSProperty aProperty,
>                                        nsStyleContext* aStyleContext,
>                                        nsStyleCoord& aComputedValue)
> {
>+  ptrdiff_t ssOffset = nsCSSProps::kStyleStructOffsetTable[aProperty];
[...]
>+      aComputedValue = static_cast<const nsStyleSides*>(
>+        StyleDataAtOffset(styleStruct, ssOffset))->
>+          Get(animType - eStyleAnimType_Sides_Top);
>+      return PR_TRUE;

After getting |ssOffset| here, we should probably assert that it's non-negative, right?  That'll help ensure we're not going to access arbitrary data in the StyleDataAtOffset call (particularly because the offset value for non-animatable properties is -1).

Likewise for the |ssOffset| variable in StoreComputedValue.

Comment 22

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

One other comment on patch 1 (in addition to comment 7): it would be good to document that ComputeDistance always returns nonnegative values; I've started depending on that.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to comment #22)
> it would be good to
> document that ComputeDistance always returns nonnegative values

Correct me if I'm wrong, but I'm assuming you're only talking about that guarantee holding *if the function succeeds* (returns PR_TRUE).  Currently, ComputeDistance could easily return a negative value on failure -- the outparam is just untouched in that case. (though we could change that)

Comment 24

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

Yeah, I meant when it returns true.  (That is, it contains the call to fabs() when that's needed.)

Comment 25

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

Attachment: patch 5: add support for animation of nsStyleCoord and nsStyleSides to nsStyleAnimation

With the assertions suggested in comment 21.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: patch 1, ver2

Here's "patch 1" (basic nsStyleAnimation patch), adding the documentation suggested in comment 22, and using a standard nsICSSStyleRule instead of a helper class as suggested in comment 7.

As part of the nsICSSStyleRule change, ComputeValue now takes a string instead of a nsCSSDeclaration, so that the declaration and the stylerule are created in the same place (since the stylerule takes ownership of the declaration).

PATCH FUZZ NOTE: dbaron's patches 2 thru 9 still stack nicely on top of this, with the following exceptions:
 (A) patches 5 and 8 require fuzz=10 when being applied over this version
 (B) patch 3 has bitrotted WRT mozilla-central, but dbaron's got a fixed version in his patch-queue: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/tip/more-cssproplist-fields

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: patch 10: Add "UncomputeValue" implementation

As noted in Comment 6, we still need an UncomputeValue function (to convert computed values back into specified values).  This patch adds that.

This patch layers on top of patch 9, and it includes support for all unit-types supported by the patches already posted here (coord, percent, color).

Comment 28

[Daniel Holbert [:dholbert]]

FWIW, I've also got patches in my patch queue to support float values (e.g. opacity) and enumerated values (e.g. 'display').

I'm not posting those here right now, to avoid more patch explosion on this bug, for the time being. :)  But for anyone interested in examining/testing them, they're here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/5fbdf9248aa5/style-animation-float
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/5fbdf9248aa5/style-animation-enum
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/5fbdf9248aa5/style-animation-none

(I've also got additional patches listed after those in my series file[1] to make SMIL use this styleanimation code.)
> [1] http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/5fbdf9248aa5/series

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 396489 [details] [diff] [review]
patch 6: add support for animation of nscoord values to nsStyleAnimation

r=dholbert on patch 6 (style-animation-nscoord)

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 396491 [details] [diff] [review]
patch 8: add support for animation of nscolor values and eStyleUnit_Color coords to nsStyleAnimation

Review comments for style-animation-color:

+++ b/layout/style/nsCSSPropList.h
>     BorderBottomColor,
[snip]
>+    // FIXME: should be animatable

It looks like 'outline' and 'border-{bottom,right,left,top}' all have these FIXME comments.  Were those just reminders to fill in the boilerplate code there, or is there more required to make those work?

>+inline PRUint8 ClampColor(PRUint32 aColor)
>+{
>+  if (aColor <= 0)
>+    return 0;

This clause isn't needed -- aColor will never be less than 0, since it's unsigned.  I'm not sure we want this exact function anyway, though -- see next comment.

>+      PRUint8 resultR =
>+        ClampColor(NS_GET_R(destColor) + aCount * NS_GET_R(colorToAdd));
[snip]
>+      aDest.SetColorValue(NS_RGBA(resultR, resultG, resultB, resultA));

This computation could overflow for high values of aCount -- e.g. if aCount is anything higher than UINT_MAX/255 - 255, we could potentially overflow back to (and past) 0, *before* we even call ClampColor.

I've got a method called "AddColorChannelAndClamp" that handles this in my "smil_css" patch, here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/558f818ac76b/smil_css#l2654

I'd lean towards doing something like that instead, to prevent this wrapping.

>+      double startA = NS_GET_A(startColor) / 255.0;
>+      double startR = NS_GET_R(startColor) * startA / 255.0;
>+      double startG = NS_GET_G(startColor) * startA / 255.0;
>+      double startB = NS_GET_B(startColor) * startA / 255.0;
[snip]
>+      aDistance = sqrt(diffA * diffA + diffR * diffR +
>+                       diffG * diffG + diffB * diffB);

Is there a reason why we need to divide by 255.0 everywhere when computing the distance?  It seems like we'd get the same result (with a larger-but-equivalent distance-scale) if we just removed all of those divisions.

Also, if we _do_ keep those, it scares me a bit to have so many copies of the string "255.0", in case we were to accidentally introduce a typo in one of them, or something.  I count 11 copies of that string in this patch.  I'd lean towards using a #define or a "static const double" at the top of the file, instead.

>+#if 0
>+      // Don't use premultiplication.
>+      aResultValue.SetColorValue(
>+        NS_RGBA(PRUint8(NS_GET_R(startColor) * inv +
>+                        NS_GET_R(endColor) * aPortion),

If you want to keep this #if'd-out section, I think it needs some NSToIntRound calls inside of each PRUint8 cast.
(But maybe you'd rather remove this section entirely, with that "#if 0" above it?)

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 396492 [details] [diff] [review]
patch 9: add support for animation of colors in paint server values to nsStyleAnimation

r=dholbert on style-animation-paint-server.  Just a few comments (nothing to change):

>@@ -436,16 +436,30 @@ nsStyleAnimation::ExtractComputedValue
>+    case eStyleAnimType_PaintServer: {
>+      const nsStyleSVGPaint &paint = *static_cast<const nsStyleSVGPaint*>(
>+        StyleDataAtOffset(styleStruct, ssOffset));
>+      // We *could* animate 'none' by treating it as rgba(0, 0, 0, 0),
>+      // but since SVG doesn't have (or really get along with) rgba()
>+      // colors, I think we're not supposed to.

'None' is actually not supposed to be interpolated, so we can just do aComputedValue.SetNoneValue() for 'none'. (once we've got support for handling the 'none' unit-type, which I add in "style-animation-none" in my patch queue)

If you're interested, here's my change to the above-quoted section in the style-animation-none patch:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/558f818ac76b/style-animation-none#l129

>+      // FIXME: However, at some point in the future, we should animate
>+      // gradients.

Agreed.  I guess we'll need nsStyleCoord to be able to hold a URI, right?

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 397927 [details] [diff] [review]
patch 5: add support for animation of nsStyleCoord and nsStyleSides to nsStyleAnimation

r=dholbert on "style-animation-style-coord", with one comment fix:

>+  /**
>+   * Sets the computed value for the given property in the given style
>+   * struct.
>+   *
>+   * @param aProperty      The property whose value we're looking up.

Needs s/looking up/setting/ in that last line there.

Comment 33

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

Comment on attachment 397934 [details] [diff] [review]
patch 1, ver2

Makefile.in:

>+               nsStyleAnimation.h \

I tend to think this header shouldn't be exported; anybody outside the
directory who needs it should just add layout/style to the
LOCAL_INCLUDES list in the makefile.

nsStyleAnimation.cpp:

>+  NS_ABORT_IF_FALSE(aDest.GetUnit() != eStyleUnit_Null,
>+                    "Expecting values with non-null units");
>+  NS_ABORT_IF_FALSE(aValueToAdd.GetUnit() != eStyleUnit_Null,
>+                    "Expecting values with non-null units");

>+  NS_ABORT_IF_FALSE(aStartValue.GetUnit() != eStyleUnit_Null,
>+                    "Expecting values with non-null units");
>+  NS_ABORT_IF_FALSE(aEndValue.GetUnit() != eStyleUnit_Null,
>+                    "Expecting values with non-null units");

These assertions wouldn't be needed if you kept the assertion in
nsStyleCoord.h (see below).

>+  // XXXdholbert TODO: Can probably share a lot of code with Interpolate
>+  // XXXdholbert Note that the "aDistance = [...];" vs "aResultValue.Set..."
>+  // line is the only difference between this and Interpolate...

I don't think this is true once you get into more complex value types,
so I think you're better off just removing the comment.  (Consider how
different the two look for colors.)

>+  PRBool changed;
>+  nsIDocument* doc = aTargetElement->GetOwnerDoc();
>+  nsCOMPtr<nsIURI> baseURI = aTargetElement->GetBaseURI();
>+  nsIURI* docURI = doc->GetDocumentURI();
>+  nsICSSLoader* cssLoader = doc->CSSLoader();
>+  nsCOMPtr<nsICSSParser> parser;

Please move these all as close to first use as possible instead of all
at the beginning of the function.

(Maybe you don't even need |docURI| and |cssLoader|; you can just use
doc->GetDocumentURI() and doc->CSSLoader() directly.)

You'd also be able to avoid repeating the three calls to RuleAbort if
you just did:

if (!declaration->InitializeEmpty() ||
    NS_FAILED(cssLoader->GetParserFor(nsnull, getter_AddRefs(parser))) ||
    NS_FAILED(parser->ParseProperty(aProperty, aSpecifiedValue, docURI,
                                    baseURI, aTargetElement->NodePrincipal(),
                                    declaration, &changed))) {
  NS_WARNING("failure in BuildDeclaration");
  declaration->RuleAbort(); // deletes declaration
  return nsnull;
}

return declaration;

+inline
+already_AddRefed<nsStyleContext>
+LookupStyleContext(nsIContent* aElement)
+{

Given the way this function works (using GetPrimaryShell), you should at
least document that ComputeValue should only be used for things that
apply to all presentations rather than things that apply to a particular
presentation.

+  NS_ABORT_IF_FALSE(aTargetElement->GetDocument(),
+                    "we should only be able to actively animate nodes that "
+                    "are in a document");

Use GetCurrentDoc() rather than GetDocument().  (GetDocument() is
deprecated in favor of GetOwnerDoc() and GetCurrentDoc().)

+  nsStyleSet* styleSet = styleContext->PresContext()->StyleSet();

Move this closer to first use (i.e., either before or after you declare
and append to ruleArray).

Also, perhaps it makes more sense to move the NS_NewCSSStyleRule call
to inside BuildDeclaration (so it becomes BuildStyleRule?), so that
you don't have to worry about adding any RuleAbort calls outside
BuildDeclaration.

Also, does ComputeValue need to care whether the property parsed
successfully or not?  Right now you're ignoring whether ParseProperty
actually parsed the property; should you be?  (Probably the simplest way
to tell is calling declaration->SlotForValue(aProperty).)  It seems like
using ComputeValue could yield weird results on things that didn't parse
successfully; you'd end up animating from the computed value that the
element had otherwise.

In the includes at the beginning of nsStyleAnimation.cpp, I tend to
think it's good practice to include nsStyleAnimation.h first:  that way
there's one file (the corresponding .cpp file) testing that each header
includes all the files it needs to in order to compile correctly.

Also, could you put a single-line comment right after the license header
of nsStyleAnimation.cpp just like you do in nsStyleAnimation.h?  But
maybe it would be better to say "Utilities for animation of computed
style values" rather than "Utility class to handle animated style
values"?

nsStyleAnimation.h:

>+#ifndef _nsStyleAnimation_h_
>+#define _nsStyleAnimation_h_

You actually aren't supposed to have an _ at the beginning; names with a
leading underscore are reserved for the implementation and standard library.

>+   * Calculates a measure of 'distance' between two values. This distance
>+   * can be used for paced interpolation in SMIL.

For what it's worth, I'm also using ComputeDistance for transitions, for
when I reverse or change an already-running transition and want to know
how much of the way through the new transition the current value is
(relative to how far the fully transitioned value is).

+   * @param   aPortion      A number in the range [0.0, 1.0) defining
+   *                        the distance of the interpolated value in the
+   *                        interval.

This can be [0.0, 1.0].

Also, it might clarify what the function does to say that it's really
just doing:
  aResultValue = (1.0 - aPortion) * aStartValue + aPortion * aEndValue

>+   * (property ID + string).  A style context is needed in case the
>+   * specified value depends on inherited style.

depends on inherited style or on the values of other properties.

(For example, "em" units depend on the value of 'font-size'.)

>+   * @param aComputedValue  The resulting computed value. (outparam)

Throughout the file, rather than sticking "(outparam)" at the end, it
might be better to put "[out]" at the beginning, either before or after
the variable name.  (Search for "[out]" in nsICSSLoader.h or
nsCSSParser.cpp for examples both ways.)

nsStyleCoord.h:

>+// XXXdholbert: Need to extend this to be able to hold the computed values of
>+// all types we'd like to animate, per bug 504652 comment 0.

No need for this comment; we can add things as we need them; it's not
really a *problem* with this code.

>   nsStyleUnit GetUnit(void) const {
>-    NS_ASSERTION(mUnit != eStyleUnit_Null, "reading uninitialized value");
>     return mUnit;
>   }

Why is this needed?  I'd prefer to leave this.


r=dbaron with those changes, although I really ought to check with sicking about the new rules for GetCurrentDoc() vs. GetOwnerDoc(), since he claims many of the uses in layout are wrong.

Comment 34

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

Comment on attachment 397934 [details] [diff] [review]
patch 1, ver2

One more comment on patch 1:

>+   * @return  PR_TRUE on success, PR_FALSE on failure.
>+   * @return  PR_TRUE on success, PR_FALSE on failure.
>+   * @result  PR_TRUE on success, or PR_FALSE on failure.

You should probably stick to @return rather than @result, and you should probably also say the same thing for ComputeValue and ExtractComputedValue.

(And, in patch 10, for UncomputeValue.  I just fixed patch 5 in my tree.)

Comment 35

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

Comment on attachment 398197 [details] [diff] [review]
patch 10: Add "UncomputeValue" implementation

nsStyleAnimation.cpp:

>+PRBool
>+nsStyleAnimation::UncomputeValue(nsCSSProperty aProperty,
>+                                 nsPresContext* aPresContext,
>+                                 const nsStyleCoord& aComputedValue,
>+                                 nsAString& aSpecifiedValue)
>+{
>+  NS_ABORT_IF_FALSE(aSpecifiedValue.IsEmpty(),
>+                    "outparam should start as an empty string");

I'd just make this aSpecifiedValue.Truncate().  It's often useful to reuse strings.

>+
>+  nsCSSValue value;
>+  switch(aComputedValue.GetUnit()) {

space before (

>+    case eStyleUnit_Coord: {
>+      if (!aPresContext) {
>+        return PR_FALSE;
>+      }

I *would* make this an NS_ABORT_IF_FALSE at the start of the function, though.

nsStyleAnimation.h:

>+   * Creates a specified value (nsAString) for the given computed value
>+   * (nsStyleCoord).  A presentation context may be needed, to convert between
>+   * app units and CSS pixels.

I'd just drop the second sentence here; the @param covers it.

>+   * @param aProperty       The property whose value we're uncomputing.
>+   * @param aPresContext    The presentation context for the document in
>+   *                        which we're working.
>+   * @param aComputedValue  The computed value to be converted.
>+   * @param aSpecifiedValue The resulting specified value. (outparam)

Same comment about "(outparam)" and @return as for patch 1.

r=dbaron with those things fixed

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: patch 1 ver3 [landed with SlotForValue fix, c45]

Thanks for the styleanimation review, dbaron!  I'm attaching updated patch.  I'll post an interdiff as well.

I've addressed all your comments, with links to changesets in my patch queue. Let me know if you have any concerns about the added nsStyleCoord::IsNull method (mentioned below), or if you think I should request additional review.  Otherwise, I think this patch is ready to land!

(In reply to comment #33)
> >+               nsStyleAnimation.h \
> 
> I tend to think this header shouldn't be exported

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce73e6f3bc44

> >+  NS_ABORT_IF_FALSE(aEndValue.GetUnit() != eStyleUnit_Null,
> >+                    "Expecting values with non-null units");
> 
> These assertions wouldn't be needed if you kept the assertion in
> nsStyleCoord.h (see below).

Fixed (but see response below comment about GetUnit):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bbb05ccd86c5

> >+  // XXXdholbert TODO: Can probably share a lot of code with Interpolate
> 
> I don't think this is true once you get into more complex value types,
> so I think you're better off just removing the comment.  (Consider how
> different the two look for colors.)

Fixed (removed comment):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce73e6f3bc44

> >+  PRBool changed;
> >+  nsIDocument* doc = aTargetElement->GetOwnerDoc();
> >+  nsCOMPtr<nsIURI> baseURI = aTargetElement->GetBaseURI();
> >+  nsIURI* docURI = doc->GetDocumentURI();
> >+  nsICSSLoader* cssLoader = doc->CSSLoader();
> >+  nsCOMPtr<nsICSSParser> parser;
> 
> Please move these all as close to first use as possible instead of all
> at the beginning of the function.

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce73e6f3bc44

> (Maybe you don't even need |docURI| and |cssLoader|; you can just use
> doc->GetDocumentURI() and doc->CSSLoader() directly.)
>
> You'd also be able to avoid repeating the three calls to RuleAbort if
> you [used a big "if" statement.]

Fixed both of these:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/82c1b1b98adc
Initially left cssLoader, but removed that here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/a8b318236f22

> Given the way this function works (using GetPrimaryShell), you should at
> least document that ComputeValue should only be used for things that
> apply to all presentations rather than things that apply to a particular
> presentation.

Fixed (added comment, with an XXX chunk mentioning that this won't matter after we get rid of multiple presentations, per IRC discussion with bz):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/9ed0769ffc02

> +  NS_ABORT_IF_FALSE(aTargetElement->GetDocument(),
> +                    "we should only be able to actively animate nodes that "
> +                    "are in a document");
> 
> Use GetCurrentDoc() rather than GetDocument().  (GetDocument() is
> deprecated in favor of GetOwnerDoc() and GetCurrentDoc().)

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/9bee25f59e85

> +  nsStyleSet* styleSet = styleContext->PresContext()->StyleSet();
> 
> Move this closer to first use (i.e., either before or after you declare
> and append to ruleArray).
>
> Also, perhaps it makes more sense to move the NS_NewCSSStyleRule call
> to inside BuildDeclaration (so it becomes BuildStyleRule?)

Fixed both of these:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/94bd98074b61

> Also, does ComputeValue need to care whether the property parsed
> successfully or not?  Right now you're ignoring whether ParseProperty
> actually parsed the property; should you be?  (Probably the simplest way
> to tell is calling declaration->SlotForValue(aProperty).)

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/968f39f52167

> In the includes at the beginning of nsStyleAnimation.cpp, I tend to
> think it's good practice to include nsStyleAnimation.h first

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce73e6f3bc44

> Also, could you put a single-line comment right after the license header
> of nsStyleAnimation.cpp just like you do in nsStyleAnimation.h?  But
> maybe it would be better to say "Utilities for animation of computed
> style values"

Fixed (and updated the comment in nsStyleAnimation.h):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce73e6f3bc44

> >+#define _nsStyleAnimation_h_
> 
> You actually aren't supposed to have an _ at the beginning

Fixed (removed leading _):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce73e6f3bc44

> >+   * Calculates a measure of 'distance' between two values. This distance
> >+   * can be used for paced interpolation in SMIL.
> 
> For what it's worth, I'm also using ComputeDistance for transitions

Fixed (removed second SMIL-specific sentence):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/50c46c04e033

> +   * @param   aPortion      A number in the range [0.0, 1.0) defining
> +   *                        the distance of the interpolated value in the
> +   *                        interval.
> 
> This can be [0.0, 1.0].

Ok.  The drawback of that is it adds a small amount of extra work for us with more complex types, if we want to guarantee that aResultValue is identical to aEndValue whenever aPortion == 1.0.  Consider the "clip" property -- if we're interpolating with from="rect(1,2,3,4)" to="auto", we'd ideally like the final value to actually be "auto", and not the equivalent "rect()" representation.  This is easy, of course -- it just requires an "if (aPortion == 1.0)" check before the actual interpolation code -- but it's a check that we need to evaluate every time we call Interpolate.  (Of course, we'll already have to handle the same thing at progress=0.0.)

Our SMIL code happens to never bother interpolating with progress == 1.0 (which is why I had the non-inclusive bound documented), but I suppose the nsStyleAnimation API should be nice and allow this.  Out of curiosity, do CSS transitions ever interpolate with progress=1.0?  If not, it'd mean we have no codepath that can test the progress=1.0 case.  I guess that doesn't matter too much, though, since it's so simple to handle.

Anwyay, fixed, & added a NS_ABORT_IF_FALSE to enforce aPortion's range:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/aa4cab8033a6

> Also, it might clarify what the function does to say that it's really
> just doing:
>   aResultValue = (1.0 - aPortion) * aStartValue + aPortion * aEndValue

Fixed (added comment) in above-linked changeset.

> >+   * (property ID + string).  A style context is needed in case the
> >+   * specified value depends on inherited style.
> 
> depends on inherited style or on the values of other properties.

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/60a52053e387

> >+   * @param aComputedValue  The resulting computed value. (outparam)
> 
> Throughout the file, rather than sticking "(outparam)" at the end, it
> might be better to put "[out]" at the beginning, either before or after
> the variable name.  (Search for "[out]" in nsICSSLoader.h or
> nsCSSParser.cpp for examples both ways.)

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bf4b6b2eab3c

> nsStyleCoord.h:
> 
> >+// XXXdholbert: Need to extend this to be able to hold the computed values of
> >+// all types we'd like to animate, per bug 504652 comment 0.
> 
> No need for this comment; we can add things as we need them; it's not
> really a *problem* with this code.

Fixed (comment removed):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/ce73e6f3bc44

> 
> >   nsStyleUnit GetUnit(void) const {
> >-    NS_ASSERTION(mUnit != eStyleUnit_Null, "reading uninitialized value");
> >     return mUnit;
> >   }
> 
> Why is this needed?  I'd prefer to leave this.

There are a few places (in SMIL code) where I'd like to assert that a nsStyleCoord's unit is eStyleUnit_Null (i.e. it's uninitialized).  Currently, I can't check that without failing the assertion in GetUnit.

To avoid modifying GetUnit(), I've now added a different method "nsStyleCoord::IsNull()" to let me explicitly check for Null-unit nsStyleCoords in places where I expect them.  Maybe that's a better way to do this.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bbb05ccd86c5
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/95adc0f0ae1e

The places I want to check for Null units are in nsSMILCSSValueType's wrappers for Interpolate, Add, and ComputeDistance.  Here's a link to one call:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/54920e0c30e6/smil_css_styleanimation#l4128
Basically, in situations with pure by-animation (no explicit "from" value), we *expect* to have an uninitialized nsStyleCoord as our "starting value", because we didn't receive any "from" string that we would otherwise be parsing. In these places, I overwrite the nsStyleCoord, and I want to assert that it's Null before I do that.

Do you think IsNull() is an acceptable solution?  I'd ideally like to be able to assert Null-ness, so I think we need _something_ like that.

(In reply to comment #34)
> One more comment on patch 1:
> >+   * @result  PR_TRUE on success, or PR_FALSE on failure.
> 
> You should probably stick to @return rather than @result, and you should
> probably also say the same thing for ComputeValue and ExtractComputedValue.

Good point -- I noticed this problem yesterday when fixing up other comments.  Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/50c46c04e033

(In reply to comment #33)
> r=dbaron with those changes

Yay, thanks for the review!

> although I really ought to check with sicking
> about the new rules for GetCurrentDoc() vs. GetOwnerDoc(), since he claims many
> of the uses in layout are wrong.

(I'm assuming this is an afterthought -- I'm pretty sure GetCurrentDoc() is correct in this case, but we can always change it later if sicking commands it. :) He's out of town right now, or else I'd just ping him in IRC)

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: interdiff: patch 1, ver2 -> ver3

Here's the interdiff for patch 1 ver2 --> ver3
(attachment 397934 [details] [diff] [review] --> attachment 398763 [details] [diff] [review])

Comment 38

[Daniel Holbert [:dholbert]]

Attachment: patch 10, ver2: Add "UncomputeValue" implementation (addressed dbaron's review comments)

(In reply to comment #35)
Thanks for the styleanimation_uncomputevalue review comments! Here's the updated patch.

Fixed everything from comment 35 in:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/cafb4d258ba3
with the exception of the "@return" documentation, which I added in:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/50c46c04e033

I'm not attaching an interdiff for this one, since the changes were few & small, and the two above-linked hg URLs capture the changes.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 397927 [details] [diff] [review]
patch 5: add support for animation of nsStyleCoord and nsStyleSides to nsStyleAnimation

sr=me, though I wish there were a way to avoid duplicating the struct names in nsCSSPropList.  :(

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 396491 [details] [diff] [review]
patch 8: add support for animation of nscolor values and eStyleUnit_Color coords to nsStyleAnimation

>+++ b/layout/style/nsStyleAnimation.cpp
>+inline PRUint8 ClampColor(PRUint32 aColor)
>+{
>+  if (aColor <= 0)
>+    return 0;

That code doesn't make sense given the type of aColor...  Is the type wrong, or the code?

>@@ -202,16 +234,42 @@ nsStyleAnimation::ComputeDistance(const 

>+      // that we should use Euclidian RGB cube distance.  However, we
....
>+      // Euclidian distance in the (part of the) 4-cube of premultiplied

"Euclidean", please.

Comment 41

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

(In reply to comment #39)
> (From update of attachment 397927 [details] [diff] [review])
> sr=me, though I wish there were a way to avoid duplicating the struct names in
> nsCSSPropList.  :(

There might be, eventually, but only once we have an member variable to use for each property, which we don't now, but we should be a bit closer to once we've made more progress here.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 396491 [details] [diff] [review]
patch 8: add support for animation of nscolor values and eStyleUnit_Color coords to nsStyleAnimation

sr- pending some resolution of the ClampColor thing.

Comment 43

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

(In reply to comment #36)
> I've addressed all your comments, with links to changesets in my patch queue.
> Let me know if you have any concerns about the added nsStyleCoord::IsNull
> method (mentioned below), or if you think I should request additional review. 

Sounds fine.

> Otherwise, I think this patch is ready to land!

Yep.

> > Also, does ComputeValue need to care whether the property parsed
> > successfully or not?  Right now you're ignoring whether ParseProperty
> > actually parsed the property; should you be?  (Probably the simplest way
> > to tell is calling declaration->SlotForValue(aProperty).)
> 
> Fixed:
> http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/968f39f52167

Could you add a comment above the SlotForValue call saying that it's checking whether the property parsed without CSS parsing errors, since it's not particularly obvious?

> > +   * @param   aPortion      A number in the range [0.0, 1.0) defining
> > +   *                        the distance of the interpolated value in the
> > +   *                        interval.
> > 
> > This can be [0.0, 1.0].
> 
> Ok.  The drawback of that is it adds a small amount of extra work for us with
> more complex types, if we want to guarantee that aResultValue is identical to
> aEndValue whenever aPortion == 1.0.  Consider the "clip" property -- if we're
> interpolating with from="rect(1,2,3,4)" to="auto", we'd ideally like the final
> value to actually be "auto", and not the equivalent "rect()" representation. 
> This is easy, of course -- it just requires an "if (aPortion == 1.0)" check
> before the actual interpolation code -- but it's a check that we need to
> evaluate every time we call Interpolate.  (Of course, we'll already have to
> handle the same thing at progress=0.0.)

I think transitions does, although I'm not sure.

That said, I also don't agree with this clip case; anything that was a problem at 1.0 would also be a problem at 0.0 if you swapped the values.  I don't think it's possible to animate to/from clip:auto, although it should eventually be possible to animate to/from clip:rect(auto,auto,auto,auto), which is different from clip:auto.

> Do you think IsNull() is an acceptable solution?  I'd ideally like to be able
> to assert Null-ness, so I think we need _something_ like that.

Again, sounds fine.

> > although I really ought to check with sicking
> > about the new rules for GetCurrentDoc() vs. GetOwnerDoc(), since he claims many
> > of the uses in layout are wrong.
> 
> (I'm assuming this is an afterthought -- I'm pretty sure GetCurrentDoc() is
> correct in this case, but we can always change it later if sicking commands it.
> :) He's out of town right now, or else I'd just ping him in IRC)

Yeah; I sent him email a few days ago asking if he'd post something to the newsgroups describing what the new rules were, but haven't heard back.  This seems right based on the old rules.

Comment 44

[Daniel Holbert [:dholbert]]

(In reply to comment #43)
> > Otherwise, I think this patch is ready to land!
>
> Yep.

Great -- I'll land when the tree re-opens.

> Could you add a comment above the SlotForValue call

Added:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5c0a0cb6b567

> That said, I also don't agree with this clip case; anything that was a problem
> at 1.0 would also be a problem at 0.0 if you swapped the values.

Right -- aPortion=0.0 and aPortion=1.0 are both edge cases, and both of them require special-case code for "special" values.  I was pointing out before that SMIL code never actually uses aPortion=1.0, so if we wanted to, we could ignore *that* special case, for the purposes of SMIL.  However, I agree that it's nicer & more symmetric to handle both endpoints, so nevermind about that. :)

> I don't think
> it's possible to animate to/from clip:auto, although it should eventually be
> possible to animate to/from clip:rect(auto,auto,auto,auto), which is different
> from clip:auto.

Fair enough.  Of course, we'll still need to make sure we handle rect(auto,auto,auto,auto) correctly, with *that* ending up as the computed value, instead of the equivalent numeric representation.

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 398763 [details] [diff] [review]
patch 1 ver3 [landed with SlotForValue fix, c45]

Patch 1 pushed: http://hg.mozilla.org/mozilla-central/rev/8f1f32186e8e
(This is attachment 398763 [details] [diff] [review] plus the added SlotForValue comment
mentioned in comment 43 & comment 44)

Comment 46

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

Attachment: patch 8 (v2): add support for animation of nscolor values and eStyleUnit_Color coords to nsStyleAnimation

I think this should address the issues in comment 30 and comment 40.

Comment 47

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

Also, as far as how to make progress from here:  I think I want to stop adding properties to nsStyleAnimation until after at least one of transitions and smil animation of CSS lands (preferably both?), so that we can add automated tests for the rest of the changes to nsStyleAnimation as we make them (and, first, add tests for each property that we already added).

Comment 48

[Daniel Holbert [:dholbert]]

Comment on attachment 399816 [details] [diff] [review]
patch 8 (v2): add support for animation of nscolor values and eStyleUnit_Color coords to nsStyleAnimation

>@@ -202,16 +241,53 @@ nsStyleAnimation::ComputeDistance(const 
>+      // Get a color component on a 0-1 scale, which is much easier to
>+      // deal with when working with alpha.
>+      #define GET_COMPONENT(component_, color_) \
>+        (NS_GET_##component_(color_) * (1.0 / 255.0))
>+
>+      double startA = GET_COMPONENT(A, startColor);

>@@ -249,16 +325,50 @@ nsStyleAnimation::Interpolate(const nsSt
>+      double startA = NS_GET_A(startColor) * (1.0 / 255.0);
[snip]
>+      double endA = NS_GET_A(endColor) * (1.0 / 255.0);

It looks like Interpolate should use GET_COMPONENT for startA and endA too. (in which case maybe GET_COMPONENT should move to the top of the file somewhere?)

r=dholbert with that

Comment 49

[Daniel Holbert [:dholbert]]

(In reply to comment #48)
> It looks like Interpolate should use GET_COMPONENT

Sorry, one other minor nit: Maybe GET_NORMALIZED_COMPONENT or GET_UNIT_COMPONENT would be a better name for this macro?  Otherwise, it's not immediately clear from the name that it'll do anything beyond NS_GET_A/R/G/B.

Comment 50

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

(In reply to comment #48)
> It looks like Interpolate should use GET_COMPONENT for startA and endA too. (in
> which case maybe GET_COMPONENT should move to the top of the file somewhere?)

(In reply to comment #49)
> Sorry, one other minor nit: Maybe GET_NORMALIZED_COMPONENT or
> GET_UNIT_COMPONENT would be a better name for this macro?  Otherwise, it's not
> immediately clear from the name that it'll do anything beyond NS_GET_A/R/G/B.

I'd rather leave it as-is so that the macro is right next to the code, and readers of Interpolate don't need to dig around to see what that macro does.

Is that ok?

Comment 51

[Daniel Holbert [:dholbert]]

(In reply to comment #50)
> Is that ok?

Sure, that's fine. [sorry for the delay -- I thought I'd responded already, but it turns out I didn't press "commit" on my earlier response :) ]

Comment 52

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

Landed patches 2 through 9:

http://hg.mozilla.org/mozilla-central/rev/c309a7438041
http://hg.mozilla.org/mozilla-central/rev/d1b659f7db49
http://hg.mozilla.org/mozilla-central/rev/a4990b27b935
http://hg.mozilla.org/mozilla-central/rev/284457eb8f58
http://hg.mozilla.org/mozilla-central/rev/cc19ea010061
http://hg.mozilla.org/mozilla-central/rev/75fcd0e1111d
http://hg.mozilla.org/mozilla-central/rev/e98277a9cec4
http://hg.mozilla.org/mozilla-central/rev/e24393a466bc

I just noticed that I typed the r=dholbert for patch 5 as r=dbaron.  Oops.

I filed bug 515919 as a followup for the FIXME related to use of packed colors in patch 8.

Comment 53

[Daniel Holbert [:dholbert]]

Landed patch 10: http://hg.mozilla.org/mozilla-central/rev/caf061617480

Comment 54

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

Added a bug number to the fixme from patch 8:
http://hg.mozilla.org/mozilla-central/rev/9bd1681cdc35

Comment 55

[Daniel Holbert [:dholbert]]

Closing this bug, since all the patches here have landed, and we've got bugs filed for all followup issues mentioned here. (see the bugs marked as depending on this one, plus bug 515919)

Comment 56

[Daniel Holbert [:dholbert]]

marking "in-testsuite+" based on the reftests & mochitests added in bug 474049 and bug 435441 (which depend on nsStyleAnimation to work correctly).