[webkit-reviews] review granted: [Bug 6868] make TransformationMatrix platform independent : [Attachment 27423] replacement patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 6868: make TransformationMatrix platform independent
https://bugs.webkit.org/show_bug.cgi?id=6868

Attachment 27423: replacement patch
https://bugs.webkit.org/attachment.cgi?id=27423&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> +
> +static double v3Length(Vector3 a) 

Extra space at the end.

> +{
> +    return sqrt((a[0] * a[0])+(a[1] * a[1]) + (a[2] * a[2]));

Missing space.

> +static void v3Combine (const Vector3 a, const Vector3 b, Vector3 result,
double ascl, double bscl) 

Extra space before the (, and at the end.

> +FloatPoint TransformationMatrix::mapPoint(const FloatPoint& p) const
>  {
> -    return rotate(rad2deg(atan2(y, x)));
> +    // If the matrix has 3D components, the z component of the result is
> +    // dropped, effectively projecting the point into the z=0 plane

There's no need to have the same comment in the header and the implementation.

> +    // If the matrix has 3D components, the z component of the result is
> +    // dropped, effectively projecting the point into the z=0 plane

Ditto (and 2-3 more places.


> +// This method returns the identity matrix if it is not invertible.
> +// Use isInvertible() before calling this if you need to know.

This comment belongs in the header.

> Index: WebCore/platform/graphics/transforms/TransformationMatrix.h
> ===================================================================

> +    void setMatrix(const Matrix4 m)
> +    {
> +	   if (m && m != m_matrix)
> +	       memcpy(m_matrix, m, 16 * sizeof(double));

sizeof(Matrix4) better, I think.

r=me with those nits addressed.