Closed Bug 769193 Opened 11 years ago Closed 11 years ago

Update css code to use primitives for transform functions


(Core :: Layout, defect)

Not set





(Reporter: dzbarsky, Assigned: dzbarsky)




(1 file, 1 obsolete file)

The spec says we should use primitives, which allow things interpolating between the various scale* and translate* functions.

Dbaron, do you care if I land this with async css animations?
Blocks: 755084
Attached patch Patch (obsolete) — Splinter Review
Seems to pass animation mochitests and my local testing.  Running on tryserver now.
Attachment #637511 - Flags: review?(dbaron)
Forgot to qref, so ignore the printfs
There should be a break after the translatez case in ToPrimitive
Attached patch PatchSplinter Review
This one should pass tests.
Attachment #637511 - Attachment is obsolete: true
Attachment #637511 - Flags: review?(dbaron)
Attachment #637576 - Flags: review?(dbaron)
Dbaron, do you mind if I fold this patch in with async animations?  It touches nsCSSValue.h so I have to rebuild half the tree whenever I push/pop it, which is every time I want to commit a new patch to my repo :(
Please don't.

If you want to avoid rebuilding when you push/pop patches, do the pushing/popping in one tree, pull from that one into another tree, and build there.
Or you could review this.  That would make my life a lot easier ;)
Comment on attachment 637576 [details] [diff] [review]

>From: David Zbarsky <>
>try: -b do

In the future, please include commit messages in patches for review.

For this one, I'd suggest as a commit message (all on one line):

Interpolate between transform functions that share common primitives rather than forcing them to fall back to matrix decomposition.  (Bug 769193)  r=dbaron

Throughout the patch, the opening brace of a function, when not
inlined in the class definition, should be on its own line.

>+  void SetItem(size_t aIndex, nsCSSValue& aValue) {
>+    (*this)[aIndex] = aValue;
>+  }

This is unneeded.  You can use either operator[] (preferable if you
have an object or reference) or Item() (preferable if you have a pointer).

So you should change:
>+      arr->SetItem(1, aArray->Item(1));
+      arr->Item(1) = aArray->Item(1);
throughout the patch.

I think it would be substantially clearer to:
 * make AppendFunction use the old numbers, i.e., the ones you removed
   from nsStyleAnimation::AppendTransformFunction
 * call ToPrimitive(nsCSSKeyword) at the start of 
 * call AppendFunction on the *result* of that first ToPrimitive call
   when setting up the result array on the second line of ToPrimitive

Remove the printf from AddTransformTranslate.

You might also want to double-check with mattwoodrow that the only time we do transform-function-based checks for whether a transform is 3-D is in the parser (which this shouldn't go through again) and that any other checks for is-it-3D are based on the matrix.  You're depending on that.

r=dbaron with that
Attachment #637576 - Flags: review?(dbaron) → review+
Addressed nits and landed:
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.