[webkit-reviews] review denied: [Bug 23689] Add support for 3D to TransformationMatrix and WebKitCSSMatrix : [Attachment 27401] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 23689: Add support for 3D to TransformationMatrix and WebKitCSSMatrix
https://bugs.webkit.org/show_bug.cgi?id=23689

Attachment 27401: Patch
https://bugs.webkit.org/attachment.cgi?id=27401&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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.