531344 - implement animation of CSS transforms in nsStyleAnimation

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

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

This bug is for implementing animation of CSS transforms in
nsStyleAnimation (i.e., support CSS transitions of CSS 2D transforms).

The relevant section of the transitions specification:
http://dev.w3.org/csswg/css3-transitions/#animation-of-property-types-
defers all of the details to the 2-D and 3-D transforms specifications.
For the 2-D transforms specification (all that's relevant for us, right
now), the relevant section is:
http://dev.w3.org/csswg/css3-2d-transforms/#animation
This, in turn, refers to the unmatrix program in Graphics Gems,
available from http://tog.acm.org/resources/GraphicsGems/ , and in
particular as the file GraphicsGems/gemsii/unmatrix.c
in http://tog.acm.org/resources/GraphicsGems/AllGems.tar.gz

The unmatrix reference is for general 3-D transform matrices (any of the
16 components can have any value).

For CSS 2-D transforms, we have a 2-D matrix with the bottom row constant:

[ A C E ]
[ B D F ]
[ 0 0 1 ]

For that case, I believe the algorithm in unmatrix reduces to:

 (1) If A * D - B * C == 0, the matrix is singular.  Fail.

 (2) Set translation components (Tx and Ty) to the translation parts of
     the matrix (E and F) and then ignore them for the rest of the time.
     (For us, E and F each actually consist of three constants:  a
     length, a multiplier for the width, and a multiplier for the
     height.  This actually requires its own decomposition, but I'll
     keep that separate.)

 (3) Let the X scale factor (Sx) be sqrt(A^2 + B^2).  Then divide both A
     and B by it.

 (4) Let the XY shear (K) be A * C + B * D.  From C, subtract A times
     the XY shear.  From D, subtract B times the XY shear.

 (5) Let the Y scale (Sy) be sqrt(C^2 + D^2).  Divide C, D, and the XY
     shear (K) by it.

 (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
     negate all of A, B, C, D, the X scale (Sx), and the Y scale (Sy).

 (7) Let the rotation be R = atan2(B, A).

Then the resulting decomposed transformation is:

  translate(Tx, Ty) rotate(R) skewX(atan(K)) scale(Sx, Sy)

An interesting result of this is that all of the simple transform
functions (i.e., all functions other than matrix()), in isolation,
decompose back to themselves except for:
  'skewY(φ)', which is 'matrix(1, tan(φ), 0, 1, 0, 0)', which decomposes
  to 'rotate(φ) skewX(φ) scale(sec(φ), cos(φ))' since (ignoring the
  alternate sign possibilities that would get fixed in step 6):
    In step 3, the X scale factor is sqrt(1+tan²(φ)) = sqrt(sec²(φ)) = sec(φ).
    Thus, after step 3, A = 1/sec(φ) = cos(φ) and B = tan(φ) / sec(φ) = sin(φ).
    In step 4, the XY shear is sin(φ).
    Thus, after step 4, C = -cos(φ)sin(φ) and D = 1 - sin²(φ) = cos²(φ).
    Thus, in step 5, the Y scale is sqrt(cos²(φ)(sin²(φ) + cos²(φ)) = cos(φ).
    Thus, after step 5, C = -sin(φ), D = cos(φ), and the XY shear is tan(φ).
    Thus, in step 6, A * D - B * C = cos²(φ) + sin²(φ) = 1.
    In step 7, the rotation is thus φ.

  skew(θ, φ), which is matrix(1, tan(φ), tan(θ), 1, 0, 0), which decomposes
  to 'rotate(φ) skewX(θ + φ) scale(sec(φ), cos(φ))' since (ignoring
  the alternate sign possibilities that would get fixed in step 6):
    In step 3, the X scale factor is sqrt(1+tan²(φ)) = sqrt(sec²(φ)) = sec(φ).
    Thus, after step 3, A = 1/sec(φ) = cos(φ) and B = tan(φ) / sec(φ) = sin(φ).
    In step 4, the XY shear is cos(φ)tan(θ) + sin(φ).
    Thus, after step 4,
    C = tan(θ) - cos(φ)(cos(φ)tan(θ) + sin(φ)) = tan(θ)sin²(φ) - cos(φ)sin(φ)
    D = 1 - sin(φ)(cos(φ)tan(θ) + sin(φ)) = cos²(φ) - sin(φ)cos(φ)tan(θ)
    Thus, in step 5, the Y scale is sqrt(C² + D²) = 
    sqrt(tan²(θ)(sin⁴(φ) + sin²(φ)cos²(φ)) -
         2 tan(θ)(sin³(φ)cos(φ) + sin(φ)cos³(φ)) +
         (sin²(φ)cos²(φ) + cos⁴(φ))) =
    sqrt(tan²(θ)sin²(φ) - 2 tan(θ)sin(φ)cos(φ) + cos²(φ)) =
    cos(φ) - tan(θ)sin(φ) (taking the negative of the obvious solution so
    we avoid flipping in step 6).
    After step 5, C = -sin(φ) and D = cos(φ), and the XY shear is
    (cos(φ)tan(θ) + sin(φ)) / (cos(φ) - tan(θ)sin(φ)) =
    (dividing both numerator and denominator by cos(φ))
    (tan(θ) + tan(φ)) / (1 - tan(θ)tan(φ)) = tan(θ + φ).
    (See http://en.wikipedia.org/wiki/List_of_trigonometric_identities .)
    Thus, in step 6, A * D - B * C = cos²(φ) + sin²(φ) = 1.
    In step 7, the rotation is thus φ.

    To check this result, we can multiply things back together:

    [ cos(φ) -sin(φ) ] [ 1 tan(θ + φ) ] [ sec(φ)    0   ]
    [ sin(φ)  cos(φ) ] [ 0      1     ] [   0    cos(φ) ]

    [ cos(φ)      cos(φ)tan(θ + φ) - sin(φ) ] [ sec(φ)    0   ]
    [ sin(φ)      sin(φ)tan(θ + φ) + cos(φ) ] [   0    cos(φ) ]

    but since tan(θ + φ) = (tan(θ) + tan(φ)) / (1 - tan(θ)tan(φ)),
    cos(φ)tan(θ + φ) - sin(φ)
     = cos(φ)(tan(θ) + tan(φ)) - sin(φ) + sin(φ)tan(θ)tan(φ)
     = cos(φ)tan(θ) + sin(φ) - sin(φ) + sin(φ)tan(θ)tan(φ)
     = cos(φ)tan(θ) + sin(φ)tan(θ)tan(φ)
     = tan(θ) (cos(φ) + sin(φ)tan(φ))
     = tan(θ) sec(φ) (cos²(φ) + sin²(φ))
     = tan(θ) sec(φ)
    and
    sin(φ)tan(θ + φ) + cos(φ)
     = sin(φ)(tan(θ) + tan(φ)) + cos(φ) - cos(φ)tan(θ)tan(φ)
     = tan(θ) (sin(φ) - sin(φ)) + sin(φ)tan(φ) + cos(φ)
     = sec(φ) (sin²(φ) + cos²(φ))
     = sec(φ)
    so the above is:
    [ cos(φ)  tan(θ) sec(φ) ] [ sec(φ)    0   ]
    [ sin(φ)     sec(φ)     ] [   0    cos(φ) ]

    [    1   tan(θ) ]
    [ tan(φ)    1   ]



So the algorithms in the spec seem largely reasonable to me.

I think it is valuable to avoid the matrix decomposition for transform
lists that match, since it will produce better results in a number of
cases, I think.

One question I wonder, though, is whether animation of skew should
animate over angle or over number (i.e., the animation function
transitions between two tangents rather than between two angles); I tend
to think it should actually be the latter.

Comment 2

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

Attachment: patch 1: store the specified transform list in nsStyleDisplay

This patch series applies on top of the patches in bug 363249 up through patch 22 (though it doesn't need patch 19 or 20 there).

Comment 3

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

Attachment: patch 2: add helper functions for some common nsCSSValue interpolations

Comment 4

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

Attachment: patch 3: add common function for setting pieces of transform matrix to a translation

Comment 5

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

Attachment: patch 4: allow CSS parser to forbid min()/max() within certain calc() expressions

For transforms, I'm going to implement calc() without min() and max(); that makes it straightforward to convert to the matrix form that we use; without that restriction we'd have to do the full transform computation after we learned the width and height.

Comment 6

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

Attachment: patch 5: fix an odd function signature in nsStyleTransformMatrix

Comment 7

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

Attachment: patch 6: add support for calc() expressions in translate functions of -moz-transform

Comment 8

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

Attachment: patch 7: move ReadTransforms from nsRuleNode to nsStyleTransformMatrix

Comment 9

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

Attachment: patch 8 (main patch): implement animation of -moz-transform property

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 454195 [details] [diff] [review]
patch 5: fix an odd function signature in nsStyleTransformMatrix

Glancing at patch 5 first, since it looks smallest. :)

>+static nscoord CalcLength(const nsCSSValue &aValue,
>+                          nsStyleContext* aContext,
>+                          nsPresContext* aPresContext,
>+                          PRBool &aCanStoreInRuleTree)
> {
>-  aOut = nsRuleNode::CalcLength(aValue, aContext, aPresContext,
>+  return nsRuleNode::CalcLength(aValue, aContext, aPresContext,
>                                 aCanStoreInRuleTree);
> }

Why do we need this method at all?  Wouldn't it be simpler for the clients of this method to just call nsRuleNode::CalcLength directly?

Comment 11

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

We don't, really, though it saves an "nsRuleNode::".  I should probably make it inline if we keep it, though.

Comment 12

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

Attachment: patch 8 (main patch): implement animation of -moz-transform property

Here's a revised version of the main patch.  The revision is to allow a small amount of floating point error to accumulate in two of the tests (one, which was previously commented out, is needed on my machine, and for the other test it's needed for Mac try).

With that change, the patches in this bug and the calc() patches they depend on are green on try server.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 454191 [details] [diff] [review]
patch 1: store the specified transform list in nsStyleDisplay

Notes for patch 1:

>     /* If we need to inherit, do so by making a full deep-copy. */
>     else if (head->mValue.GetUnit() == eCSSUnit_Inherit)  {
>-      display->mTransformPresent = parentDisplay->mTransformPresent;
>-      if (parentDisplay->mTransformPresent)
>+      display->mSpecifiedTransform = parentDisplay->mSpecifiedTransform;
>+      if (parentDisplay->mSpecifiedTransform)

As we just spoke about, I think this comment needs updating (sine the code doesn't actually do a deep-copy).  Otherwise, looks good; r=dholbert

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 454192 [details] [diff] [review]
patch 2: add helper functions for some common nsCSSValue interpolations

r=dholbert on patch 2

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 454193 [details] [diff] [review]
patch 3: add common function for setting pieces of transform matrix to a translation

r=dholbert on patch 3

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 454195 [details] [diff] [review]
patch 5: fix an odd function signature in nsStyleTransformMatrix

(In reply to comment #11)
> We don't, really, though it saves an "nsRuleNode::".  I should probably make it
> inline if we keep it, though.

r=dholbert if you make the CalcLength helper-function inline.  You also should probably update the comment above it to mention something like "Convenience wrapper for RuleNode::CalcLength".

(or if you prefer, this patch could just use direct calls to nsRuleNode::CalcLength everywhere -- r=dholbert either way)

Comment 17

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

(In reply to comment #16)
> r=dholbert if you make the CalcLength helper-function inline.  You also should
> probably update the comment above it to mention something like "Convenience
> wrapper for RuleNode::CalcLength".
> 
> (or if you prefer, this patch could just use direct calls to
> nsRuleNode::CalcLength everywhere -- r=dholbert either way)

Actually, the reason it's not inline is that it becomes more complicated in the main patch in this bug.  (I just explained that to dholbert verbally, and he was ok with it.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 454198 [details] [diff] [review]
patch 7: move ReadTransforms from nsRuleNode to nsStyleTransformMatrix

r=dholbert on patch 7

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 454333 [details] [diff] [review]
patch 8 (main patch): implement animation of -moz-transform property

>diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp
>--- a/layout/style/nsStyleAnimation.cpp
>+++ b/layout/style/nsStyleAnimation.cpp
>+      double squareDistance = 0.0f;

Nit: I don't think we need/want the "f" suffix there, since it's getting stored in a double. (applies to a few other places in this file, too)

>+AppendTransformFunction(nsCSSKeyword aTransformFunction,

Could you add an assertion to check that aTransformFunction is a valid transform function? (and not some arbitrary nsCSSKeyword)

>+  nsRefPtr<nsCSSValue::Array> arr = nsCSSValue::Array::Create(nargs + 1);
>+  if (!arr) {
>+    return nsnull;
>+  }

No need to null-check 'arr' here -- nsCSSValue::Array::Create uses infallible-new under the hood. (or, if you'd prefer to keep the null-check to allow for ::Create to be fallible in the future, then there are a few not-null-checked ::Create calls elsewhere in this patch that would need checking.)

>+  nsCSSValueList *item = new nsCSSValueList;
>+  if (!item) {
>+    return nsnull;
>+  }

No need to null-check 'item'.

>+ *  (3) Let the X scale factor (Sx) be sqrt(A^2 + B^2).  Then divide both A
>+ *      and B by it.
[...]
>+ *  (5) Let the Y scale (Sy) be sqrt(C^2 + D^2).  Divide C, D, and the XY
>+ *      shear (K) by it.

Nit: s/Y scale/Y scale factor/, for consistency with "X scale factor" in (3). (or, remove "factor" from (3))

>+ *  (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
>+ *      negate the XY shear (K) and the Y scale (Sy).
[...]
>+  if (A * D < B * C) { // A*D - B*C should be 1 or -1
>+    A = -A;
>+    B = -B;
>+    C = -C;
>+    D = -D;
>+    XYshear = -XYshear;
>+    scaleX = -scaleX;
>+  }

The code here doesn't match the documentation (quoted above). In particular:
 - The documentation says to negate the *Y* scale, but the code negate the *X* scale instead.
 - The documentation doesn't mention negating A/B/C/D, and yet the code negates them (which I think has no effect in the case of C & D, since those variables aren't used after this block).

Can you fix the code & documentation so they're in sync on the above points?

Also, could you add an assertion or an NS_ABORT_IF_FALSE just above this block to verify that your "A*D - B*C should be 1 or -1" claim holds? (within some epsilon of float error)

>+      arr = AppendTransformFunction(tfunc, resultTail);
>+      if (!arr) {
>+        return nsnull;
>+      }

Given the null-check-removals that I suggested above, I think AppendTransformFunction as a whole is infallible.  So I don't think you need to null-check its return value here.  (There are other places in the patch where you already don't null-check its return value.)

>+AddTransformLists(const nsCSSValueList* aList1, double aCoeff1,
[...]
>+        nsCSSValue zero(0.0f, eCSSUnit_Pixel);
[...]
>+        nsCSSValue zero(0.0f, eCSSUnit_Radian);

Looks like these |zero| values are never modified -- can we be declare them as |static const| so that we don't have to re-initialize them every time we traverse this code?


>+        AddTransformTranslate(a1->Count() == 3 ? a1->Item(2) : zero,
>+                              aCoeff1,
>+                              a2->Count() == 3 ? a2->Item(2) : zero,
>+                              aCoeff2,
>+                              arr->Item(2));
>+        AddTransformTranslate(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
>+                              arr->Item(1));

The ordering here (handling 'y' before 'x') seems a bit counterintuitive -- seems to me like it would make more sense to handle 'x' before 'y'.

Regardless of the ordering, though, could you add comments above each of the AddTransformTranslate calls to say:
  // add x component of translation
and
  // add y component of translation
(With the 1/2/3/zero/a1/a2/etc values and no mention of 'x' or 'y', it takes a bit of work to parse what's going on right now, at first glance.)


Both of these points apply to the "scale" and "skew" cases in the same switch statement.


>@@ -872,16 +1447,86 @@ nsStyleAnimation::AddWeighted(nsCSSPrope
[...]
>+          result = new nsCSSValueList;
>+          if (result) {
>+            result->mValue.SetNoneValue();
>+          }

No need to null-check |result| here.

>+      if (!result) {
>+        return PR_FALSE;
>+      }

(This is at the end of the Transform clause inside of "AddWeighted")
Do we actually need this check? That is, can |result| actually be null here?  It looks to me like it gets set to something non-null in every single case.  But I might be missing something.

>+SubstitutePixelValues(nsStyleContext* aStyleContext,
[...]
>+    PRBool canStoreInRuleTree = PR_TRUE;
>+    nscoord len = nsRuleNode::CalcLength(aInput, aStyleContext,
>+                                         aStyleContext->PresContext(),
>+                                         canStoreInRuleTree);

I'd get rid of the "PR_TRUE" initialization there.  |canStoreInRuleTree| is an outparam (which you're ignoring), so it's a bit misleading to initialize it to something. (Mostly for consistency -- elsewhere in this patch, you pass a boolean named "dummy" as an |aCanStoreInRuleTree| outparam, and you don't initialize |dummy| to anything.)

>@@ -1666,16 +2342,51 @@ nsStyleAnimation::ExtractComputedValue(n
>+              nsCSSValueList *clone = new nsCSSValueList;
>+              if (!clone) {
>+                result = nsnull;
>+                break;
>+              }

No need to null-check |clone| here.

>+            result = new nsCSSValueList();
>+            if (result) {
>+              result->mValue.SetNoneValue();

No need to null-check |result| here.

>+          if (!result) {
>+            NS_WARNING("out of memory");
>+            return PR_FALSE;
>+          }

I don't think we could ever hit this OOM clause (right?), so it could probably be removed or converted into an NS_ABORT_IF_FALSE.

>+++ b/layout/style/nsStyleTransformMatrix.cpp
[...]
> /* Helper function to fill in an nscoord with the specified nsCSSValue. */
> static nscoord CalcLength(const nsCSSValue &aValue,
[...]
>+    // fine) so that callers aare allowed to pass a null style context

s/aare/are/

>+  /**
>+   * Return the transform function, as an nsCSSKeyword, for the given
>+   * nsCSSValue::Array from a transform list.
>+   */
>+/* static */ nsCSSKeyword
>+nsStyleTransformMatrix::TransformFunctionOf(const nsCSSValue::Array* aData)
>+{

That header comment is indented 2 spaces here (probably from being copied from .h-->.cpp file) -- nix the indentation here.

>+++ b/layout/style/nsStyleTransformMatrix.h
>+   * aContext and aPresContext may be null if all of the length values in
>+   * aData are already known to have been converted to eCSSUnit_Pixel (as
>+   * they are in an nsStyleAnimation::Value)
>    */
>   void SetToTransformFunction(const nsCSSValue::Array* aData,

To avoid ambiguity, it's probably worth indicating in the comment above that percent values don't need to have been converted. Perhaps replace "length values" with "non-percent length values", or with "length values (not percents)"?

You do have comments elsewhere about percent-values being OK, but those comments are both inside the .cpp file (in ExtractComputedValue and in your static CalcLength() function, to be specific), and it seems like this is something worth documenting in the header file.

r=dholbert with the above fixed/addressed.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 454194 [details] [diff] [review]
patch 4: allow CSS parser to forbid min()/max() within certain calc() expressions

r=bzbarsky

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 454197 [details] [diff] [review]
patch 6: add support for calc() expressions in translate functions of -moz-transform

r=bzbarsky

Comment 22

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

(In reply to comment #19)
> >+AddTransformLists(const nsCSSValueList* aList1, double aCoeff1,
> [...]
> >+        nsCSSValue zero(0.0f, eCSSUnit_Pixel);
> [...]
> >+        nsCSSValue zero(0.0f, eCSSUnit_Radian);
> 
> Looks like these |zero| values are never modified -- can we be declare them as
> |static const| so that we don't have to re-initialize them every time we
> traverse this code?

I'd actually prefer not to prematurely optimize in this case, especially for an optimization that has an even odds chance of being slower rather than faster.

> >+        AddTransformTranslate(a1->Count() == 3 ? a1->Item(2) : zero,
> >+                              aCoeff1,
> >+                              a2->Count() == 3 ? a2->Item(2) : zero,
> >+                              aCoeff2,
> >+                              arr->Item(2));
> >+        AddTransformTranslate(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
> >+                              arr->Item(1));
> 
> The ordering here (handling 'y' before 'x') seems a bit counterintuitive --
> seems to me like it would make more sense to handle 'x' before 'y'.

I did it because the compiler is probably more likely to merge with the case below (which can be done non-DEBUG only) if the code is in that order, I think.

> Regardless of the ordering, though, could you add comments above each of the
> AddTransformTranslate calls to say:
>   // add x component of translation
> and
>   // add y component of translation
> (With the 1/2/3/zero/a1/a2/etc values and no mention of 'x' or 'y', it takes a
> bit of work to parse what's going on right now, at first glance.)

Yeah, I'll add comments, and mention the reason for the order.

> I'd get rid of the "PR_TRUE" initialization there.  |canStoreInRuleTree| is an
> outparam (which you're ignoring), so it's a bit misleading to initialize it to
> something. (Mostly for consistency -- elsewhere in this patch, you pass a
> boolean named "dummy" as an |aCanStoreInRuleTree| outparam, and you don't
> initialize |dummy| to anything.)

It's not a true out-param; it's a param that gets false written to it but never gets true written to it.  I'd sort of prefer to leave the initialization.

I have everything else addressed locally.

Comment 23

[Daniel Holbert [:dholbert]]

Thanks, that sounds good.

RE the pseudo-out-param -- maybe consider whether that logic applies to |dummy|, too (which currently isn't initialized to anything, and gets passed into nsStyleTransformMatrix::ReadTransforms). Maybe that one's more of a 'true' outparam, though, and that's the difference?  Anyway, I trust your judgement on that, of course. :)

Comment 24

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

I think the difference is that I don't care what value a variable called |dummy| has, but if it has a sensible name it ought to have a sensible value, and the code is a little clearer when it has a sensible name.

Comment 25

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

http://hg.mozilla.org/mozilla-central/rev/c88af82d34cc
http://hg.mozilla.org/mozilla-central/rev/fd83cf94ce08
http://hg.mozilla.org/mozilla-central/rev/b2fea2529876
http://hg.mozilla.org/mozilla-central/rev/4125a0ac5ec5
http://hg.mozilla.org/mozilla-central/rev/15e9cd490443
http://hg.mozilla.org/mozilla-central/rev/aa23d71ab230
http://hg.mozilla.org/mozilla-central/rev/e419e47c09bf
http://hg.mozilla.org/mozilla-central/rev/f2b02ba56bdd

That should put it in tomorrow's mozilla-central nightly (July 3), and also in Firefox 4.0 beta 2 (but not beta 1).

Comment 26

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

And if you notice it crashing, that's probably bug 576761; I'll try to get that fix in for tomorrow's nightly.

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to comment #19)
> >+ *  (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
> >+ *      negate the XY shear (K) and the Y scale (Sy).
> [...]
> >+  if (A * D < B * C) { // A*D - B*C should be 1 or -1
> >+    A = -A;
> >+    B = -B;
> >+    C = -C;
> >+    D = -D;
> >+    XYshear = -XYshear;
> >+    scaleX = -scaleX;
> >+  }
> 
> The code here doesn't match the documentation (quoted above). In particular:
>  - The documentation says to negate the *Y* scale, but the code negate the *X*
> scale instead.
>  - The documentation doesn't mention negating A/B/C/D, and yet the code negates
> them (which I think has no effect in the case of C & D, since those variables
> aren't used after this block).
> 
> Can you fix the code & documentation so they're in sync on the above points?

Looks like this point wasn't fixed in the final push [1] (the first half seeming more important, about Sx vs Sy) -- mind investigating & fixing whichever of the code vs. documentation is incorrect?

[1] http://hg.mozilla.org/mozilla-central/rev/f2b02ba56bdd

Comment 28

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

The final push had:

   2.247 + *  (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
   2.248 + *      negate the XY shear (K), the X scale (Sy), and A, B, C, and D.
   2.249 + *      (Alternatively, we could negate the XY shear (K) and the Y scale
   2.250 + *      (Sy).)

In other words, we have a choice between:
 (1) negating K, Sx, A, B, (C, D)
 (2) negating K, Sy
Since the spec isn't clear (due to the problems described in http://lists.w3.org/Archives/Public/www-style/2010Jun/0602.html ) I just picked one.

The final push does have a single typo, though:  "X scale (Sy)" instead of "X scale (Sx)".

Comment 29

[Daniel Holbert [:dholbert]]

Ah, sorry for overstating it -- I'd just glanced at the comment and saw the Sy, and then saw that the code part hadn't changed & still had scaleX. (but I missed that the rest of the comment had changed).

So, yeah, just a s/Sy/Sx/ is all that's needed, looks like.  Thanks!

Comment 30

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

Comment fixed:
http://hg.mozilla.org/mozilla-central/rev/70967e44b56c