Bug 23689 – Add support for 3D to TransformationMatrix and WebKitCSSMatrix

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Marrin]

The current CSS Transform proposal specifies 3D functions in WebKitCSSMatrix. In talking to Darin, he sees no problem having this 3D support in WebKit separately from support of 3D transforms in CSS.

Here is a link to the 2D version (which is currently in TOT):
 
    http://webkit.org/specs/CSSVisualEffects/CSSTransforms.html#cssmatrix-interface

and here is a link to the 3D version:

    http://webkit.org/specs/CSSVisualEffects/CSSTransforms3D.html#cssmatrix-interface

Attached is a composite IDL encompassing the functionality of both. I believe we will replace the two in the current specs with this one.

[Chris Marrin]

Created attachment 27253 [details]
IDL of composite 2D/3D CSSMatrix

[Chris Marrin]

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

[Simon Fraser (smfr)]

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.

[Darin Adler]

(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.

[Chris Marrin]

Created attachment 27509 [details]
Replacement patch

Replacement patch addressing all issues

[Chris Marrin]

Created attachment 27510 [details]
Replacement patch addressing one more issue

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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