520234 - Extend nsStyleAnimation to support mixing absolute lengths with percent values

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 2

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

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.

Comment 3

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

Attachment: patch 1: add calc() value type

Comment 4

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

Attachment: patch 2: extract calc() when needed

Comment 5

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

Attachment: patch 3: add property bit for which properties have stored calc()

Comment 6

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

Attachment: patch 4: use calc() for interpolation of simple values

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

Comment 7

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

Attachment: patch 5: use calc() for interpolation of value pairs and value pair lists

This patch does have tests.

Comment 8

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

Attachment: patch 2: extract calc() when needed

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.

Comment 9

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

Attachment: patch 4: use calc() for interpolation of simple values

with tests

Comment 10

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

Attachment: patch 5: use calc() for interpolation of value pairs and value pair lists

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 12

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Whoops, should have been r+.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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 15

[Boris Zbarsky [:bzbarsky]]

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 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 475146 [details] [diff] [review]
patch 4: use calc() for interpolation of simple values

r=me.  Very nice!

Comment 17

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

Attachment: patch 1: add calc() value type

addressing comment 11 and comment 14

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

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

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

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

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

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

Comment 24

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

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

Comment 25

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

Attachment: patch 6: add fallible array creation

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

Comment 26

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

Attachment: patch 7: remove null checks

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

Comment 29

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

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

Comment 30

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

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