769193 - Update css code to use primitives for transform functions

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Zbarsky (:dzbarsky)]

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

Attachment: Patch

Seems to pass animation mochitests and my local testing.  Running on tryserver now.

Comment 2

[David Zbarsky (:dzbarsky)]

Forgot to qref, so ignore the printfs

Comment 3

[David Zbarsky (:dzbarsky)]

There should be a break after the translatez case in ToPrimitive

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Patch

This one should pass tests.

Comment 5

[David Zbarsky (:dzbarsky)]

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-5, no longer working on Mozilla)]

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

Or you could review this.  That would make my life a lot easier ;)

Comment 8

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

Comment on attachment 637576 [details] [diff] [review]
Patch

>From: David Zbarsky <[e-mail]>
>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)]

Addressed nits and landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b5733f954d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19b5733f954d