The default bug view has changed. See this FAQ.

Update css code to use primitives for transform functions

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 755084
(Assignee)

Comment 1

5 years ago
Created attachment 637511 [details] [diff] [review]
Patch

Seems to pass animation mochitests and my local testing.  Running on tryserver now.
Attachment #637511 - Flags: review?(dbaron)
(Assignee)

Comment 2

5 years ago
Forgot to qref, so ignore the printfs
(Assignee)

Comment 3

5 years ago
There should be a break after the translatez case in ToPrimitive
(Assignee)

Comment 4

5 years ago
Created attachment 637576 [details] [diff] [review]
Patch

This one should pass tests.
Attachment #637511 - Attachment is obsolete: true
Attachment #637511 - Flags: review?(dbaron)
Attachment #637576 - Flags: review?(dbaron)
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Or you could review this.  That would make my life a lot easier ;)
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
Attachment #637576 - Flags: review?(dbaron) → review+
(Assignee)

Comment 9

5 years ago
Addressed nits and landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b5733f954d
Flags: in-testsuite+
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/19b5733f954d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.