Closed Bug 520234 Opened 10 years ago Closed 9 years ago

Extend nsStyleAnimation to support mixing absolute lengths with percent values

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: dholbert, Assigned: dbaron)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 4 obsolete files)

15.30 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
14.35 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
25.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.50 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
14.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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...
OS: Linux → All
Hardware: x86 → All
Duplicate of this bug: 536539
Assignee: nobody → dbaron
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.
blocking2.0: --- → beta6+
Attached patch patch 1: add calc() value type (obsolete) — Splinter Review
Attachment #474986 - Flags: review?(bzbarsky)
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.)
Attachment #474989 - Flags: review?(bzbarsky)
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.
Attachment #474987 - Attachment is obsolete: true
Attachment #475145 - Flags: review?(bzbarsky)
Attachment #474987 - Flags: review?(bzbarsky)
with tests
Attachment #474989 - Attachment is obsolete: true
Attachment #475146 - Flags: review?(bzbarsky)
Attachment #474989 - Flags: review?(bzbarsky)
I missed adding a case in StyleCoordToCSSValue.  As a result of that case, I also made it propagate errors.
Attachment #474990 - Attachment is obsolete: true
Attachment #475147 - Flags: review?(bzbarsky)
Attachment #474990 - Flags: review?(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"?
Attachment #474986 - Flags: review?(bzbarsky) → review-
Comment on attachment 474988 [details] [diff] [review]
patch 3: add property bit for which properties have stored calc()

r=me
Attachment #474988 - Flags: review?(bzbarsky) → review-
Comment on attachment 474988 [details] [diff] [review]
patch 3: add property bit for which properties have stored calc()

Whoops, should have been r+.
Attachment #474988 - Flags: review- → review+
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.
Attachment #475145 - Flags: review?(bzbarsky) → review+
Comment on attachment 475146 [details] [diff] [review]
patch 4: use calc() for interpolation of simple values

r=me.  Very nice!
Attachment #475146 - Flags: review?(bzbarsky) → review+
addressing comment 11 and comment 14
Attachment #474986 - Attachment is obsolete: true
Attachment #475227 - Flags: review?(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 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.
Attachment #475147 - Flags: review?(bzbarsky) → review+
(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
Attachment #475227 - Flags: review?(bzbarsky) → review+
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.
Attachment #475262 - Flags: review?(bzbarsky)
And this removes the unneeded null checks, both from the patches above and those already in the tree.
Attachment #475263 - Flags: review?(bzbarsky)
Comment on attachment 475262 [details] [diff] [review]
patch 6: add fallible array creation

r=me
Attachment #475262 - Flags: review?(bzbarsky) → review+
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
Attachment #475263 - Flags: review?(bzbarsky) → review+
(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).
Blocks: 598208
Duplicate of this bug: 597361
See Also: → 1218257
Blocks: 1218257
See Also: 1218257
You need to log in before you can comment on or make changes to this bug.