Status
()
People
(Reporter: dbaron, Assigned: dbaron)
Tracking
(Blocks: 1 bug)
Firefox Tracking Flags
(blocking2.0 beta2+)
Details
Attachments
(8 attachments, 1 obsolete attachment)
9.20 KB,
patch

dholbert
:
review+

Details  Diff  Splinter Review 
7.98 KB,
patch

dholbert
:
review+

Details  Diff  Splinter Review 
4.84 KB,
patch

dholbert
:
review+

Details  Diff  Splinter Review 
4.93 KB,
patch

bzbarsky
:
review+

Details  Diff  Splinter Review 
3.51 KB,
patch

dholbert
:
review+

Details  Diff  Splinter Review 
12.08 KB,
patch

bzbarsky
:
review+

Details  Diff  Splinter Review 
7.80 KB,
patch

dholbert
:
review+

Details  Diff  Splinter Review 
57.48 KB,
patch

dholbert
:
review+

Details  Diff  Splinter Review 
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/css3transitions/#animationofpropertytypes defers all of the details to the 2D and 3D transforms specifications. For the 2D transforms specification (all that's relevant for us, right now), the relevant section is: http://dev.w3.org/csswg/css32dtransforms/#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 3D transform matrices (any of the 16 components can have any value). For CSS 2D transforms, we have a 2D 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.
blocking2.0:  → ?
(Assignee)  
Comment 2•9 years ago


Created attachment 454191 [details] [diff] [review] 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).
Attachment #454191 
Flags: review?(dholbert)
(Assignee)  
Comment 3•9 years ago


Created attachment 454192 [details] [diff] [review] patch 2: add helper functions for some common nsCSSValue interpolations
Attachment #454192 
Flags: review?(dholbert)
(Assignee)  
Comment 4•9 years ago


Created attachment 454193 [details] [diff] [review] patch 3: add common function for setting pieces of transform matrix to a translation
Attachment #454193 
Flags: review?(dholbert)
(Assignee)  
Comment 5•9 years ago


Created attachment 454194 [details] [diff] [review] 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.
Attachment #454194 
Flags: review?(bzbarsky)
(Assignee)  
Comment 6•9 years ago


Created attachment 454195 [details] [diff] [review] patch 5: fix an odd function signature in nsStyleTransformMatrix
Attachment #454195 
Flags: review?(dholbert)
(Assignee)  
Comment 7•9 years ago


Created attachment 454197 [details] [diff] [review] patch 6: add support for calc() expressions in translate functions of moztransform
Attachment #454197 
Flags: review?(bzbarsky)
(Assignee)  
Comment 8•9 years ago


Created attachment 454198 [details] [diff] [review] patch 7: move ReadTransforms from nsRuleNode to nsStyleTransformMatrix
Attachment #454198 
Flags: review?(dholbert)
(Assignee)  
Comment 9•9 years ago


Created attachment 454199 [details] [diff] [review] patch 8 (main patch): implement animation of moztransform property
Attachment #454199 
Flags: review?(dholbert)
Comment 10•9 years ago


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?
(Assignee)  
Comment 11•9 years ago


We don't, really, though it saves an "nsRuleNode::". I should probably make it inline if we keep it, though.
(Assignee)  
Comment 12•9 years ago


Created attachment 454333 [details] [diff] [review] patch 8 (main patch): implement animation of moztransform 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.
Attachment #454199 
Attachment is obsolete: true
Attachment #454333 
Flags: review?(dholbert)
Attachment #454199 
Flags: review?(dholbert)
Comment 13•9 years ago


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 deepcopy. */ > 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 deepcopy). Otherwise, looks good; r=dholbert
Attachment #454191 
Flags: review?(dholbert) → review+
Comment 14•9 years ago


Comment on attachment 454192 [details] [diff] [review] patch 2: add helper functions for some common nsCSSValue interpolations r=dholbert on patch 2
Attachment #454192 
Flags: review?(dholbert) → review+
Comment 15•9 years ago


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
Attachment #454193 
Flags: review?(dholbert) → review+
Updated•9 years ago

Attachment #454195 
Flags: review?(dholbert) → review+
Comment 16•9 years ago


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 helperfunction 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)
(Assignee)  
Comment 17•9 years ago


(In reply to comment #16) > r=dholbert if you make the CalcLength helperfunction 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•9 years ago


Comment on attachment 454198 [details] [diff] [review] patch 7: move ReadTransforms from nsRuleNode to nsStyleTransformMatrix r=dholbert on patch 7
Attachment #454198 
Flags: review?(dholbert) → review+
Comment 19•9 years ago


Comment on attachment 454333 [details] [diff] [review] patch 8 (main patch): implement animation of moztransform 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 nullcheck 'arr' here  nsCSSValue::Array::Create uses infalliblenew under the hood. (or, if you'd prefer to keep the nullcheck to allow for ::Create to be fallible in the future, then there are a few notnullchecked ::Create calls elsewhere in this patch that would need checking.) >+ nsCSSValueList *item = new nsCSSValueList; >+ if (!item) { >+ return nsnull; >+ } No need to nullcheck '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 nullcheckremovals that I suggested above, I think AppendTransformFunction as a whole is infallible. So I don't think you need to nullcheck its return value here. (There are other places in the patch where you already don't nullcheck 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 reinitialize 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 nullcheck 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 nonnull 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 nullcheck clone here. >+ result = new nsCSSValueList(); >+ if (result) { >+ result>mValue.SetNoneValue(); No need to nullcheck 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 "nonpercent length values", or with "length values (not percents)"? You do have comments elsewhere about percentvalues 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.
Attachment #454333 
Flags: review?(dholbert) → review+
Comment 20•9 years ago


Comment on attachment 454194 [details] [diff] [review] patch 4: allow CSS parser to forbid min()/max() within certain calc() expressions r=bzbarsky
Attachment #454194 
Flags: review?(bzbarsky) → review+
Comment 21•9 years ago


Comment on attachment 454197 [details] [diff] [review] patch 6: add support for calc() expressions in translate functions of moztransform r=bzbarsky
Attachment #454197 
Flags: review?(bzbarsky) → review+
(Assignee)  
Comment 22•9 years ago


(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 reinitialize 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 nonDEBUG 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 outparam; 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•9 years ago


Thanks, that sounds good. RE the pseudooutparam  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. :)
(Assignee)  
Comment 24•9 years ago


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.
(Assignee)  
Comment 25•9 years ago


http://hg.mozilla.org/mozillacentral/rev/c88af82d34cc http://hg.mozilla.org/mozillacentral/rev/fd83cf94ce08 http://hg.mozilla.org/mozillacentral/rev/b2fea2529876 http://hg.mozilla.org/mozillacentral/rev/4125a0ac5ec5 http://hg.mozilla.org/mozillacentral/rev/15e9cd490443 http://hg.mozilla.org/mozillacentral/rev/aa23d71ab230 http://hg.mozilla.org/mozillacentral/rev/e419e47c09bf http://hg.mozilla.org/mozillacentral/rev/f2b02ba56bdd That should put it in tomorrow's mozillacentral nightly (July 3), and also in Firefox 4.0 beta 2 (but not beta 1).
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Priority:  → P2
Resolution:  → FIXED
Target Milestone:  → mozilla1.9.3b2
(Assignee)  
Comment 26•9 years ago


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•9 years ago


(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/mozillacentral/rev/f2b02ba56bdd
(Assignee)  
Comment 28•9 years ago


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/wwwstyle/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•9 years ago


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!
blocking2.0: ? → beta2+
(Assignee)  
Comment 30•9 years ago


Comment fixed: http://hg.mozilla.org/mozillacentral/rev/70967e44b56c
You need to log in
before you can comment on or make changes to this bug.
Description
•