Currently, nsStyleAnimation's Interpolate/Add/ComputeDistance methods can all handle two absolute lengths, or two percent values. But if you pass them one absolute-length and one percent value, they fail. dbaron proposed a partial solution to this: Use CSS calc() (once it's implemented) to do the work for us. * nsStyleAnimation::Interpolate can effectively return: "calc((1.0 - aPortion) * aStartValue + aPortion * aEndValue)". * nsStyleAnimation::Add can effectively return: "calc(aDest + aCount * aValueToAdd)" For ComputeDistance, however, I don't think calc() will help us. ComputeDistance needs to return some numeric distance-measure, and in order to come up with that, I think we'll still need to convert our input-values into common units...

This is the easiest way to fix bug 522142, which is a blocker, and since this is a feature (even though bug 522142 isn't), it blocks beta6.

This still needs tests, which I plan to write tomorrow. (The next patch has tests, though, so I'm reasonably confident of this one, though it wouldn't surprise me if I have to adjust some assertions or maybe a little more.)

This patch does have tests.

The tests for patch 4 found bugs in patches 2 and 5, but not in patch 4. Here's the revised patch 2, with two errors in the new case in StyleCoordToValue fixed.

with tests

I missed adding a case in StyleCoordToCSSValue. As a result of that case, I also made it propagate errors.

Comment on attachment 474986 [details] [diff] [review] patch 1: add calc() value type >+++ b/layout/style/nsStyleAnimation.cpp >@@ -208,16 +246,23 @@ nsStyleAnimation::ComputeDistance(nsCSSP >+ aDistance = sqrt(v1.mLength * v1.mLength + v1.mPercent * v1.mPercent + >+ v2.mLength * v2.mLength + v2.mPercent * v2.mPercent); That doesn't make any sense to me. Ignoring for the moment the relative weighting of length and percent issue, there should be some subtractions like |v1.mLength - v2.mLength| somewhere here. >+nsStyleAnimation::Value::SetAndAdoptCSSValueValue(nsCSSValue *aValue, >+ NS_ABORT_IF_FALSE(aValue != nsnull, "value pairs may not be null"); "value pairs"?

Comment on attachment 474988 [details] [diff] [review] patch 3: add property bit for which properties have stored calc() r=me

Comment on attachment 474988 [details] [diff] [review] patch 3: add property bit for which properties have stored calc() Whoops, should have been r+.

Comment on attachment 474986 [details] [diff] [review] patch 1: add calc() value type Also, is Array::Create fallible? The result is checked for arr but not arr2.

Comment on attachment 475145 [details] [diff] [review] patch 2: extract calc() when needed >+++ b/layout/style/nsStyleAnimation.cpp >+SetCalcValue(const nsStyleCoord::Calc* aCalc, nsCSSValue& aValue) >+ if (!arr2) >+ return false; Doesn't that leak arr? >+ arr2->Item(0).SetFloatValue( >+ nsPresContext::AppUnitsToFloatCSSPixels(aCalc->mLength), eCSSUnit_Pixel); Would it make sense to move nscoordToCSSValue higher up in the file and use it here? r=me with at least the leak addressed.

Comment on attachment 475146 [details] [diff] [review] patch 4: use calc() for interpolation of simple values r=me. Very nice!

addressing comment 11 and comment 14

Comment on attachment 475227 [details] [diff] [review] patch 1: add calc() value type Doesn't this leak arr if !arr2? I guess there's no really good way to decide on relative scaling for the length and %, so equal weighting is as good as anything.....

Comment on attachment 475147 [details] [diff] [review] patch 5: use calc() for interpolation of value pairs and value pair lists >@@ -300,41 +351,50 @@ nsStyleAnimation::ComputeDistance(nsCSSP >+ aDistance = sqrt(v1.mLength * v1.mLength + >+ v1.mPercent * v1.mPercent + >+ v2.mLength * v2.mLength + >+ v2.mPercent * v2.mPercent); This really doesn't make sense; first of all this is the same bogus-looking expression as in the earlier patch, and second you want to be setting diff, not aDistance, right? That applies to both value pairs and value pair lists. >+AddCSSValueCanonicalCalc(double aCoeff1, const nsCSSValue &aValue1, This doesn't null-check |a| and |acalc|. Should it? It would be nice to share the duplicated code here with the simple value case, perhaps... Either way, though. r=me with the null-check sorted out.

(In reply to comment #18) > Comment on attachment 475227 [details] [diff] [review] > patch 1: add calc() value type > > Doesn't this leak arr if !arr2? No, since the SetArrayValue call is before the null-check.

Comment on attachment 475227 [details] [diff] [review] patch 1: add calc() value type > No, since the SetArrayValue call is before the null-check. Ah, indeed. r=me

Oh, and do we need tests for those distance computations?

(In reply to comment #19)> Comment on attachment 475147 [details] [diff] [review]> >+AddCSSValueCanonicalCalc(double aCoeff1, const nsCSSValue &aValue1,> > This doesn't null-check |a| and |acalc|. Should it?As discussed, I'm actually going to leave this, and try to make Create infalliable in most cases.

Actually, Create is already infalliable (though it should probably have a falliable version).

I though this was fallible, but it was actually infallible. This converts the two callers that want it to be fallible to be fallible.

And this removes the unneeded null checks, both from the patches above and those already in the tree.

Comment on attachment 475262 [details] [diff] [review] patch 6: add fallible array creation r=me

Comment on attachment 475263 [details] [diff] [review] patch 7: remove null checks >+++ b/layout/style/nsMediaFeatures.cpp > MakeArray(const nsSize& aSize, nsCSSValue& aResult) This can be a void function now, right? r=me

(In reply to comment #28) > Comment on attachment 475263 [details] [diff] [review] > patch 7: remove null checks > > >+++ b/layout/style/nsMediaFeatures.cpp > > MakeArray(const nsSize& aSize, nsCSSValue& aResult) > > This can be a void function now, right? > > r=me It could be, except it's a helper for functions that are required to return nsresult, so I figured it was better to leave the return; it might help compilers optimize better (either by tail call elimination or by complete function elimination).

http://hg.mozilla.org/mozilla-central/rev/6e67db3fa0f0 http://hg.mozilla.org/mozilla-central/rev/56a2166d2bd9 http://hg.mozilla.org/mozilla-central/rev/5a5ea460027b http://hg.mozilla.org/mozilla-central/rev/576e1b23a27b http://hg.mozilla.org/mozilla-central/rev/91d20b5e47d8 http://hg.mozilla.org/mozilla-central/rev/6b4bc4c7d2b5 http://hg.mozilla.org/mozilla-central/rev/ffa5e7bea1e9 http://hg.mozilla.org/mozilla-central/rev/cfa340639ce6