Last Comment Bug 769193 - Update css code to use primitives for transform functions
: Update css code to use primitives for transform functions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
Depends on:
Blocks: 755084
  Show dependency treegraph
 
Reported: 2012-06-28 02:51 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-07-20 21:04 PDT (History)
5 users (show)
dzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (15.08 KB, patch)
2012-06-28 07:39 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Patch (15.72 KB, patch)
2012-06-28 10:25 PDT, David Zbarsky (:dzbarsky)
dbaron: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2012-06-28 02:51:08 PDT
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?
Comment 1 David Zbarsky (:dzbarsky) 2012-06-28 07:39:12 PDT
Created attachment 637511 [details] [diff] [review]
Patch

Seems to pass animation mochitests and my local testing.  Running on tryserver now.
Comment 2 David Zbarsky (:dzbarsky) 2012-06-28 07:40:58 PDT
Forgot to qref, so ignore the printfs
Comment 3 David Zbarsky (:dzbarsky) 2012-06-28 09:26:04 PDT
There should be a break after the translatez case in ToPrimitive
Comment 4 David Zbarsky (:dzbarsky) 2012-06-28 10:25:36 PDT
Created attachment 637576 [details] [diff] [review]
Patch

This one should pass tests.
Comment 5 David Zbarsky (:dzbarsky) 2012-07-03 21:59:17 PDT
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 :(
Comment 6 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-07-03 22:15:28 PDT
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.
Comment 7 David Zbarsky (:dzbarsky) 2012-07-03 22:40:11 PDT
Or you could review this.  That would make my life a lot easier ;)
Comment 8 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-07-19 19:27:57 PDT
Comment on attachment 637576 [details] [diff] [review]
Patch

>From: David Zbarsky <dzbarsky@gmail.com>
>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));
to:
+      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 
   ToPrimitive(nsCSSValue::Array*)
 * 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
Comment 9 David Zbarsky (:dzbarsky) 2012-07-20 11:10:01 PDT
Addressed nits and landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b5733f954d
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:04:35 PDT
https://hg.mozilla.org/mozilla-central/rev/19b5733f954d

Note You need to log in before you can comment on or make changes to this bug.