Summary: | Add support for 3D to TransformationMatrix and WebKitCSSMatrix | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Marrin <cmarrin> | ||||||||||
Component: | CSS | Assignee: | Chris Marrin <cmarrin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dino, gilles, ml, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | 6868 | ||||||||||||
Bug Blocks: | 23359 | ||||||||||||
Attachments: |
|
Description
Chris Marrin
2009-02-02 12:21:06 PST
Created attachment 27253 [details]
IDL of composite 2D/3D CSSMatrix
Created attachment 27401 [details] Patch Added 3D functions to WebKitCSSMatrix. This depends on the 3D functions added to TransformationMatrix in https://bugs.webkit.org/show_bug.cgi?id=6868 Comment on attachment 27401 [details] Patch > Index: WebCore/ChangeLog > =================================================================== > + * css/WebKitCSSMatrix.cpp: > + (WebCore::WebKitCSSMatrix::translate): > + (WebCore::WebKitCSSMatrix::scale): > + (WebCore::WebKitCSSMatrix::rotate): > + (WebCore::WebKitCSSMatrix::rotateAxisAngle): > + (WebCore::WebKitCSSMatrix::toString): > + * css/WebKitCSSMatrix.h: > + (WebCore::WebKitCSSMatrix::a): > + (WebCore::WebKitCSSMatrix::b): > + (WebCore::WebKitCSSMatrix::c): > + (WebCore::WebKitCSSMatrix::d): > + (WebCore::WebKitCSSMatrix::e): ... It's OK to remove useless lines from the changes files/functions list when they don't add any useful info. > Index: WebCore/css/WebKitCSSMatrix.cpp > =================================================================== > +PassRefPtr<WebKitCSSMatrix> WebKitCSSMatrix::translate(double x, double y, double z) > { > + // Passing a NaN (or undefined in JavaScript) will use default values > + // This allows the 3D form to used for 2D operations This comment should be in the header, and maybe the idl? > if (isnan(x)) > x = 0; Does isnan() work on all platforms? > + if (isnan(z)) > + z = 0; > + return WebKitCSSMatrix::create(m_matrix.translate3d(x, y, z)); This seems wrong; it will change |this|. > + // Passing a NaN (or undefined in JavaScript) will use default values > + // This allows the 3D form to used for 2D operations > + // In the case of scale, passing scaleX or scaleZ as NaN uses 1, but > + // scaleY of NaN makes it the same as scaleX Ditto. > + // Passing a NaN (or undefined in JavaScript) will use default values > + // This allows the 3D form to used for 2D operations > + // In the case of rotate, if rotY and rotZ are NaN, it rotates about Z. > + // Otherwise use a rotation value of 0. Ditto. > String WebKitCSSMatrix::toString() > { > - return String::format("matrix(%f, %f, %f, %f, %f, %f)", > - m_matrix.a(), m_matrix.b(), m_matrix.c(), m_matrix.d(), m_matrix.e(), m_matrix.f()); > + if (m_matrix.isAffine()) > + return String::format("matrix(%f, %f, %f, %f, %f, %f)", > + m_matrix.a(), m_matrix.b(), m_matrix.c(), m_matrix.d(), m_matrix.e(), m_matrix.f()); > + else > + return String::format("matrix3d(%f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f)", > + m_matrix.m11(), m_matrix.m12(), m_matrix.m13(), m_matrix.m14(), > + m_matrix.m21(), m_matrix.m22(), m_matrix.m23(), m_matrix.m24(), > + m_matrix.m31(), m_matrix.m32(), m_matrix.m33(), m_matrix.m34(), > + m_matrix.m41(), m_matrix.m42(), m_matrix.m43(), m_matrix.m44()); > } Please add a FIXME comment pointing to https://bugs.webkit.org/show_bug.cgi?id=20674 > Index: WebCore/css/WebKitCSSMatrix.h > =================================================================== > void setMatrixValue(const String& string, ExceptionCode&); > > // this = this * secondMatrix > PassRefPtr<WebKitCSSMatrix> multiply(WebKitCSSMatrix* secondMatrix); > > PassRefPtr<WebKitCSSMatrix> inverse(ExceptionCode&); > - PassRefPtr<WebKitCSSMatrix> translate(float x, float y); > - PassRefPtr<WebKitCSSMatrix> scale(float scaleX, float scaleY); > - PassRefPtr<WebKitCSSMatrix> rotate(float rot); > + PassRefPtr<WebKitCSSMatrix> translate(double x, double y, double z); > + PassRefPtr<WebKitCSSMatrix> scale(double scaleX, double scaleY, double scaleZ); > + PassRefPtr<WebKitCSSMatrix> rotate(double rotX, double rotY, double rotZ); > + PassRefPtr<WebKitCSSMatrix> rotateAxisAngle(double x, double y, double z, double angle); I'd like to more comments here (in particular, being very clear about the order of operations, on |this| being modified, and on what the return value points to. > Index: WebCore/css/WebKitCSSMatrix.idl > =================================================================== > + attribute double a; ... > + attribute double m11; Needs a comment explaining why both a-f and m11-m44 are exposed, and what a-f contain when the matrix has 3d in it. > + void setMatrixValue(in DOMString string) raises (DOMException); > WebKitCSSMatrix multiply(in WebKitCSSMatrix secondMatrix); > WebKitCSSMatrix inverse() raises (DOMException); > - WebKitCSSMatrix translate(in float x, in float y); > - WebKitCSSMatrix scale(in float scaleX, in float scaleY); > - WebKitCSSMatrix rotate(in float rot); > + WebKitCSSMatrix translate(in double x, in double y, in double z); > + WebKitCSSMatrix scale(in double scaleX, in double scaleY, in double scaleZ); > + WebKitCSSMatrix rotate(in double rotX, in double rotY, in double rotZ); > + WebKitCSSMatrix rotateAxisAngle(in double x, in double y, in double z, in double angle); > > [DontEnum] DOMString toString(); These need copious comments for the documentation folks. Should use [Immutable]. Should make the order of operations very clear. Before making this wholesale float -> double conversion, we need to find out if it's OK. > Index: LayoutTests/ChangeLog > =================================================================== > + Since the CSS parser does not yet handle any of the 3D > + functions for -webkit-transform, tests for these features > + are currently commented out. Don't talk about future features. Just say that these are tests for 3D functionality of CSSMatrix. > Index: LayoutTests/transforms/3d/cssmatrix-3d-interface.xhtml > =================================================================== We should also test * the immutability of CSSMatrix * order of operations * round-tripping through a string (as far as we can). r- for now, as I think we have a bit of research to do before this can land. (In reply to comment #3) > Does isnan() work on all platforms? I believe it does as long as you include <wtf/MathExtras.h>, not just <math.h>. Also, no else after return. Created attachment 27509 [details]
Replacement patch
Replacement patch addressing all issues
Created attachment 27510 [details]
Replacement patch addressing one more issue
Comment on attachment 27510 [details] Replacement patch addressing one more issue > Index: WebCore/css/WebKitCSSMatrix.cpp > =================================================================== > +PassRefPtr<WebKitCSSMatrix> WebKitCSSMatrix::multiply(WebKitCSSMatrix* secondMatrix) const > { > TransformationMatrix tmp(m_matrix); > tmp.multiply(secondMatrix->m_matrix); We should probably null-check secondMatrix here. r=me with that fix. Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog M LayoutTests/transforms/2d/cssmatrix-interface-expected.txt M LayoutTests/transforms/2d/cssmatrix-interface.xhtml M WebCore/ChangeLog M WebCore/css/WebKitCSSMatrix.cpp M WebCore/css/WebKitCSSMatrix.h M WebCore/css/WebKitCSSMatrix.idl Committed r40807 |