[webkit-reviews] review denied: [Bug 6868] make TransformationMatrix platform independent : [Attachment 27399] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 6 10:01:45 PST 2009


Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 27399: Patch
https://bugs.webkit.org/attachment.cgi?id=27399&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================

> +	   NOTE: This patch is only tested on Mac. Other platforms will likely
have build issues.
> +	   I will fix these on Windows, but I can't test on the other
platforms.

I don't think you need to say this in the changelog.

> Index: WebCore/platform/graphics/cg/TransformationMatrixCG.cpp
> ===================================================================

> +TransformationMatrix::operator CGAffineTransform() const
>  {
> -    return CGAffineTransformConcat(m_transform, CGAffineTransform(m2));
> +    return CGAffineTransformMake(narrowPrecisionToCGFloat(m_matrix[0][0]),
> +				    narrowPrecisionToCGFloat(m_matrix[0][1]),
> +				    narrowPrecisionToCGFloat(m_matrix[1][0]),
> +				    narrowPrecisionToCGFloat(m_matrix[1][1]),
> +				    narrowPrecisionToCGFloat(m_matrix[3][0]),
> +				    narrowPrecisionToCGFloat(m_matrix[3][1]));
>  }

It's a shame that extracting the correct elements from the 4x4 matrix to make
an affine matrix happens in more than one place. Can we define an simple
affine matrix type (typedef of float array), and return that from the shared
code?

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

> +////////////////////////////// Supporting Math Functions
///////////////////////////////////////
> +//
> +// This is a set of function from various places (attributed inline) to do
things like
> +// inversion and decomposition of a 4x4 matrix. They are used throughout the
code
> +//
>
+//////////////////////////////////////////////////////////////////////////////
//////////////////

This is not standard WebCore commenting style.

> +//
> +// Adapted from Matrix Inversion by Richard Carling, Graphics Gems
<http://tog.acm.org/GraphicsGems/index.html>.
> +
> +// EULA: The Graphics Gems code is copyright-protected. In other words, you
cannot claim the text of the code 
> +// as your own and resell it. Using the code is permitted in any program,
product, or library, non-commercial 
> +// or commercial. Giving credit is not required, though is a nice gesture.
The code comes as-is, and if there 
> +// are any flaws or problems with any Gems code, nobody involved with Gems -
authors, editors, publishers, or 
> +// webmasters - are to be held responsible. Basically, don't be a jerk, and
remember that anything free comes 
> +// with no guarantee.

Someone (Darin?) needs to confirm that it's OK to add this code.

> +typedef double Vector4[4];
> +typedef double Vector3[3];
> +
> +#define SMALL_NUMBER    1.e-8

Use a constant.

> +
> +//	 inverse( original_matrix, inverse_matrix )

Nit: no spaces next to the parens.

> +static double det2x2(double a, double b, double c, double d)

For this (and other functions), I think the abbreviations don't help any.
Call it determinant2x2() or computeDeterminant2x2().

>  {
> -    TransformationMatrix m(matrix);
> +    double ans;
> +    ans = a * d - b * c;
> +    return ans;
> +}

No need for the 'ans' variable. Inline it?

> +//  double = det3x3(  a1, a2, a3, b1, b2, b3, c1, c2, c3 )
> +//  
> +//  calculate the determinant of a 3x3 matrix
> +//  in the form
> +// 
> +//	   | a1,  b1,  c1 |
> +//	   | a2,  b2,  c2 |
> +//	   | a3,  b3,  c3 |

Nit: sentence case.

> +static double det4x4(const TransformationMatrix::Matrix4& m)
> +{
> +    double ans;
> +    double a1, a2, a3, a4, b1, b2, b3, b4, c1, c2, c3, c4, d1, d2, d3, d4;
> +
> +    // assign to individual variable names to aid selecting
> +    // correct elements
> +
> +    a1 = m[0][0]; b1 = m[0][1]; 
> +    c1 = m[0][2]; d1 = m[0][3];
> +
> +    a2 = m[1][0]; b2 = m[1][1]; 
> +    c2 = m[1][2]; d2 = m[1][3];
> +
> +    a3 = m[2][0]; b3 = m[2][1]; 
> +    c3 = m[2][2]; d3 = m[2][3];
> +
> +    a4 = m[3][0]; b4 = m[3][1]; 
> +    c4 = m[3][2]; d4 = m[3][3];
> +
> +    ans = a1 * det3x3(b2, b3, b4, c2, c3, c4, d2, d3, d4)
> +	   - b1 * det3x3(a2, a3, a4, c2, c3, c4, d2, d3, d4)
> +	   + c1 * det3x3(a2, a3, a4, b2, b3, b4, d2, d3, d4)
> +	   - d1 * det3x3(a2, a3, a4, b2, b3, b4, c2, c3, c4);
> +    return ans;

No need for the 'ans' variable. Just return the result.

> +static void adjoint(const TransformationMatrix::Matrix4& srcMatrix,
TransformationMatrix::Matrix4& outMatrix)

The 'out' prefix is not webkit style, alas. You could call the variable
'result'.

> +// Returns false if the matrix is not invertible
> +static bool inverse(const TransformationMatrix::Matrix4& srcMatrix,
TransformationMatrix::Matrix4& outMatrix)

Ditto.

> +// Multiply a homogeneous point by a matrix and return the transformed point

> +static void v4MulPointByMatrix(Vector4 pin, const
TransformationMatrix::Matrix4& m, Vector4 pout)

'pout'?

Expand 'Mul' to Multiply. Is the v4 prefix necessary?

> +static double v3Length(Vector3 a) 
>  {
> -    return det() != 0.0;
> +    return sqrt((a[0] * a[0])+(a[1] * a[1])+(a[2] * a[2]));

Spaces around the + please.

> +static void v3Scale(Vector3 v, double newlen) 
>  {

What is 'newlen'? Needs a better name.

> -    return (*this) *= other;
> +    double len = v3Length(v);
> +    if (len != 0.0) {

Just (len != 0)

I don't really like that this function changes the vector in-place, while the
others like
v3Length do no. I'd prefer to see 'const' used to makek things clearer.

> +// Make a linear combination of two vectors and return the result.
> +// result = (a * ascl) + (b * bscl)
> +static void v3Combine (Vector3 a, Vector3 b, Vector3 result, double ascl,
double bscl) 
> +{
> +    result[0] = (ascl * a[0]) + (bscl * b[0]);
> +    result[1] = (ascl * a[1]) + (bscl * b[1]);
> +    result[2] = (ascl * a[2]) + (bscl * b[2]);
> +}

Ditto; use 'const'?

> +// Return the cross product c = a cross b */
> +static void v3Cross(Vector3 a, Vector3 b, Vector3 c)
> +{
> +    c[0] = (a[1] * b[2]) - (a[2] * b[1]);
> +    c[1] = (a[2] * b[0]) - (a[0] * b[2]);
> +    c[2] = (a[0] * b[1]) - (a[1] * b[0]);
> +}

Ditto.

> +static bool decompose(const TransformationMatrix::Matrix4& mat,
TransformationMatrix::DecomposedType& ret)

Use 'result' rather than 'ret' for consistency.

> +{
> +    int i, j;

Declare where used?

> +    TransformationMatrix::Matrix4 locmat;
> +    memcpy(locmat, mat, sizeof(TransformationMatrix::Matrix4));

Can't you just assign? Avoid abbreviated names, use interCaps.

> +    // Normalize the matrix.
> +    if (locmat[3][3] == 0)
> +	   return false;
> +
> +    for (i = 0; i < 4; i++)
> +	   for (j = 0; j < 4; j++)
> +	       locmat[i][j] /= locmat[3][3];
> +
> +    // pmat is used to solve for perspective, but it also provides
> +    // an easy way to test for singularity of the upper 3x3 component.
> +    TransformationMatrix::Matrix4 pmat;
> +    memcpy(pmat, locmat, sizeof(TransformationMatrix::Matrix4));

Just assign, or copy-construct?

> +    for (i = 0; i < 3; i++)
> +	   pmat[i][3] = 0;
> +    pmat[3][3] = 1;
> +
> +    if (det4x4(pmat) == 0.0)
> +	   return false;

Use == 0
 
> +    // First, isolate perspective.  This is the messiest.
> +    if (locmat[0][3] != 0 || locmat[1][3] != 0 || locmat[2][3] != 0) {
> +	   // prhs is the right hand side of the equation.
> +	   Vector4 prhs;
> +	   prhs[0] = locmat[0][3];
> +	   prhs[1] = locmat[1][3];
> +	   prhs[2] = locmat[2][3];
> +	   prhs[3] = locmat[3][3];
> +
> +	   // Solve the equation by inverting pmat and multiplying
> +	   // prhs by the inverse.  (This is the easiest way, not
> +	   // necessarily the best.)
> +	   // inverse function (and det4x4, above) from the Matrix
> +	   // Inversion gem in the first volume.

Comment seems out of place.

> +	   TransformationMatrix::Matrix4 invpmat, tinvpmat;

Don't like these variable names. Just because it's math code doesn't mean it
has
to have short names.

> +	   // Stuff the answer away.

No need for this comment.

> +    // Now, get the rotations out, as described in the gem.
> +    
> +    // FIXME - We return rotation values as quaternions because they are
easier
> +    // to recompose later. But when we add decomposition to CSSMatrix we
will 
> +    // want to return the rotation values as rx, ry, and rz values because
they 
> +    // are much easier to deal with for the author. The commented out code
below 
> +    // does that. Ultimately we will want to pass in a flag to choose which
to
> +    // return.

Don't put statements about the future into comments. They always get out of
date.
A FIXME comment is more appropriate; even better if there's a bug to track it.

> +    double T, S, x, y, z, w;

What is the significance of the uppercase?

> +// Copied from Core Animation

Ouch. Need to check if it's OK to put this here.

> +static void slerp(double qa[4], const double qb[4], double t)

Needs a better function name.

> +//////////////////////////// End of Supporting Math Functions
//////////////////////////////////

Not the normal comment style.


> +TransformationMatrix TransformationMatrix::affineTransform() const
> +{
> +    // Note that this throws away the z-axis
> +    // FIXME: indicate when this loses data somehow
> +    TransformationMatrix transform(m_matrix[0][0], m_matrix[0][1],
m_matrix[1][0], m_matrix[1][1], m_matrix[3][0], m_matrix[3][1]);
> +    return transform;

Maybe it should have a bool* param that is filled with true or false if the
caller
requests it?

> +FloatPoint TransformationMatrix::projectPoint(const FloatPoint& p) const
> +    // This is basically raytracing. We have a point in the destination
> +    // plane with z=0, and we cast a ray parallel to the z-axis from that
> +    // point to find the z-position at which it intersects the z=0 plane
> +    // with the transform applied. Once we have that point we apply the
> +    // inverse transform to find the corresponding point in the source
> +    // space.
> +    // 
> +    // Given a plane with normal Pn, and a ray starting at point R0 and
> +    // with direction defined by the vector Rd, we can find the
> +    // intersection point as a distance d from R0 in units of Rd by:
> +    // 
> +    // d = -dot (Pn', R0) / dot (Pn', Rd)
> +    
> +    double x = p.x();
> +    double y = p.y();
> +    double z = -(m13() * x + m23() * y + m43()) / m33();
> +
> +    double outX = x * m11() + y * m21() + z * m31() + m41();
> +    double outY = x * m12() + y * m22() + z * m32() + m42();
> +
> +    double w = x * m14() + y * m24() + z * m34() + m44();
> +    if (w != 1 && w != 0) {
> +	   outX /= w;
> +	   outY /= w;
> +    }
> +
> +    return FloatPoint(static_cast<float>(outX), static_cast<float>(outY));
>  }

Is it OK to put this code into open source too?

> +FloatPoint TransformationMatrix::mapPoint(const FloatPoint& p) const
> +    double x, y;
> +    multVecMatrix(p.x(), p.y(), x, y);
> +    return FloatPoint(static_cast<float>(x), static_cast<float>(y));
>  }

What does this do if the matrix is 3D?

>  IntPoint TransformationMatrix::mapPoint(const IntPoint& point) const
>  {
> -    double x2, y2;
> -    map(point.x(), point.y(), &x2, &y2);
> +    double x, y;
> +    multVecMatrix(point.x(), point.y(), x, y);
>  
>      // Round the point.
> -    return IntPoint(lround(x2), lround(y2));
> +    return IntPoint(lround(x), lround(y));
>  }

What if the matrix is 3D?

> -FloatPoint TransformationMatrix::mapPoint(const FloatPoint& point) const
> +IntRect TransformationMatrix::mapRect(const IntRect &rect) const
>  {
> -    double x2, y2;
> -    map(point.x(), point.y(), &x2, &y2);
> +    return enclosingIntRect(mapRect(FloatRect(rect)));
> +}
>  
> -    return FloatPoint(static_cast<float>(x2), static_cast<float>(y2));
> +FloatRect TransformationMatrix::mapRect(const FloatRect& r) const
> +{
> +    FloatQuad resultQuad = mapQuad(FloatQuad(r));
> +    return resultQuad.boundingBox();
>  }
>  
> -FloatQuad TransformationMatrix::mapQuad(const FloatQuad& quad) const
> +FloatQuad TransformationMatrix::mapQuad(const FloatQuad& q) const
>  {
> -    // FIXME: avoid 4 seperate library calls. Point mapping really needs
> -    // to be platform-independent code.
> -    return FloatQuad(mapPoint(quad.p1()),
> -			mapPoint(quad.p2()),
> -			mapPoint(quad.p3()),
> -			mapPoint(quad.p4()));
> +    FloatQuad result;
> +    result.setP1(mapPoint(q.p1()));
> +    result.setP2(mapPoint(q.p2()));
> +    result.setP3(mapPoint(q.p3()));
> +    result.setP4(mapPoint(q.p4()));
> +    return result;
>  }

What if the matrix is 3D for all these?

> +TransformationMatrix& TransformationMatrix::rotate3d(double x, double y,
double z, double angle)
> +{
> +    // angles are in degrees. Switch to radians
> +    angle = angle / 360.0 * M_PI * 2.0;
> +    
> +    angle /= 2.0f;
> +    double sinA = sin(angle);
> +    double cosA = cos(angle);
> +    double sinA2 = sinA * sinA;
> +    
> +    // normalize
> +    double length = sqrt(x*x+y*y+z*z);

Spaces around the operators please.

> +TransformationMatrix& TransformationMatrix::rotate3d(double rx, double ry,
double rz)
> +{
> +    // angles are in degrees. Switch to radians
> +    rx = rx / 360.0 * M_PI * 2.0;
> +    ry = ry / 360.0 * M_PI * 2.0;
> +    rz = rz / 360.0 * M_PI * 2.0;

Now there are 4 of them. Add an inline function for degreesToRadians().

> +TransformationMatrix& TransformationMatrix::skew(double sx, double sy)
> +{
> +    // angles are in degrees. Switch to radians
> +    sx = sx / 360.0 * M_PI * 2.0;
> +    sy = sy / 360.0 * M_PI * 2.0;

Here too.


> +//
> +// multiplies matrix by given matrix on the right
> +//

Comment is potentially confusing. Must clearer would just be to say:

// this = mat * this

> +TransformationMatrix& TransformationMatrix::multLeft(const
TransformationMatrix& mat)
> +{
> +    double tmp[4][4];

Just use TransformationMatrix::Matrix4 to avoid the nasty cast below:

> +    setMatrix(reinterpret_cast<double*>(tmp));
> +    return *this;
> +}

> +void TransformationMatrix::multVecMatrix(double inx, double iny, double&
outx, double& outy) const

Avoid 'in', 'out' prefixes.

> +void TransformationMatrix::multVecMatrix(double inx, double iny, double inz,
double& outx, double& outy, double& outz) const

Ditto.

> +//
> +// returns inverse of matrix
> +//

Comment doesn't add anything.

> +TransformationMatrix TransformationMatrix::inverse() const 
> +{
> +    TransformationMatrix invMat;
> +    
> +    bool inverted = WebCore::inverse(m_matrix, invMat.m_matrix);
> +    if (!inverted) {
> +	   // FIXME: indicate failure somehow
> +	   return TransformationMatrix();

Address the FIXME?

> +static inline void blendFloat(double& from, double to, double progress)
> +{
> +    if (from != to)
> +	   from = from + (to - from) * progress;
> +}
>  

> +void TransformationMatrix::blend(const TransformationMatrix& from, double
progress)

Should blend stuff really be in here?

> +void TransformationMatrix::recompose(const DecomposedType& decomp)
> +{
> +    makeIdentity();
> +    
> +    // first apply perspective
> +    m_matrix[0][3] = (float) decomp.perspectiveX;
> +    m_matrix[1][3] = (float) decomp.perspectiveY;
> +    m_matrix[2][3] = (float) decomp.perspectiveZ;
> +    m_matrix[3][3] = (float) decomp.perspectiveW;
> +    
> +    // now translate
> +    translate3d((float) decomp.translateX, (float) decomp.translateY,
(float) decomp.translateZ);
> +    
> +    // apply rotation
> +    float xx, xy, xz, xw, yy, yz, yw, zz, zw;
> +
> +    xx = (float) (decomp.quaternionX * decomp.quaternionX);
> +    xy = (float) (decomp.quaternionX * decomp.quaternionY);
> +    xz = (float) (decomp.quaternionX * decomp.quaternionZ);
> +    xw = (float) (decomp.quaternionX * decomp.quaternionW);
> +    yy = (float) (decomp.quaternionY * decomp.quaternionY);
> +    yz = (float) (decomp.quaternionY * decomp.quaternionZ);
> +    yw = (float) (decomp.quaternionY * decomp.quaternionW);
> +    zz = (float) (decomp.quaternionZ * decomp.quaternionZ);
> +    zw = (float) (decomp.quaternionZ * decomp.quaternionW);

Why is this using floats, not doubles?

> +    TransformationMatrix u(1 - 2 * (yy + zz), 2 * (xy - zw), 2 * (xz + yw),
0, 
> +			    2 * (xy + zw), 1 - 2 * (xx + zz), 2 * (yz - xw), 0,

> +			    2 * (xz - yw), 2 * (yz + xw), 1 - 2 * (xx + yy), 0,

> +			    0, 0, 0, 1);

Maybe a comment explaining what the heck that math is doing?

Longer variable name than 'u' please.

> Index: WebCore/platform/graphics/transforms/TransformationMatrix.h
> ===================================================================
> --- WebCore/platform/graphics/transforms/TransformationMatrix.h      
(revision 40698)
> +++ WebCore/platform/graphics/transforms/TransformationMatrix.h      
(working copy)
> @@ -27,21 +27,7 @@
>  #define TransformationMatrix_h
>  
>  #if PLATFORM(CG)
> -#include <CoreGraphics/CGAffineTransform.h>

> +typedef struct CGAffineTransform CGAffineTransform;

Rather than the typedef, just keep the include?

>  class TransformationMatrix {
>  public:

> +    TransformationMatrix() { makeIdentity(); }
> +    TransformationMatrix(const TransformationMatrix& t) { *this = t; }

How about m_matrix = t.m_matrix ?


> +    TransformationMatrix& operator =(const TransformationMatrix &t)
> +    {
> +	   setMatrix(reinterpret_cast<const double*>(t.m_matrix));

Nasty cast. Can we have a private setter that takes a
TransformationMatrix::Matrix4?

> +    // This form preserves the double math from input to output
> +    void map(double x, double y, double *x2, double *y2) const {
multVecMatrix(x, y, *x2, *y2); }

Use references for the result params, I think.

> +    // Map a 2D point through the transform, returning a 2D point.
> +    // Note that this ignores the z component.

What does 'ignores the z component' really mean?

> +    void reset() { makeIdentity(); }

Do we need both reset and makeIdentity?

> +
> +    // this = this * mat
> +    TransformationMatrix& multiply(const TransformationMatrix& t) { return
*this *= t; }

Rather than making use of operator*, would be a little easier to grok with use
of multLeft.

> +
> +    // this = mat * this
> +    TransformationMatrix& multLeft(const TransformationMatrix& mat);
> +    
>      TransformationMatrix& scale(double);
>      TransformationMatrix& scale(double sx, double sy);
> +    TransformationMatrix& scale3d(double sx, double sy, double sz);
>      TransformationMatrix& scaleNonUniform(double sx, double sy);

How do scale() and scaleNonUniform() differ?

> +    TransformationMatrix& rotate3d(double x, double y, double z, double
angle);

Should have a comment about normalizing x,y,z

>      TransformationMatrix& flipX();
>      TransformationMatrix& flipY();

No flipZ()?

>      bool isInvertible() const;
>      TransformationMatrix inverse() const;

Why is this not invert() (non-const), when all the other methods change |this|?


> +    TransformationMatrix& operator*=(const TransformationMatrix& t)
> +    {
> +	   *this = *this * t;
> +	   return *this;
> +    }

A "this is a multRight" comment might be good here.

> +    
> +    TransformationMatrix operator*(const TransformationMatrix& t)

Should be |const|


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

I think this should go away.

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 40718)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,67 @@
> +2009-02-06  Chris Marrin  <cmarrin at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=6868
> +
> +	   These tests started failing when I changed TransformationMatrix
> +	   to be platform independent. This is due to slight differences
> +	   in rounding because I am now using doubles rather than floats
> +	   to do the matrix operations.

Rather than the "story-telling" type changelog, I think it's better to just
say "fix the tests because ...."

> Index:
LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-30-t-expected.txt
> ===================================================================
> --- LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-30-t-expected.txt  
(revision 40698)
> +++ LayoutTests/platform/mac/svg/W3C-SVG-1.1/animate-elem-30-t-expected.txt  
(working copy)
> @@ -9,7 +9,7 @@ layer at (0,0) size 480x360
>	 RenderPath {polyline} at (211.66,216.46) size 49.87x59.99
[transform={m=((0.97,0.26)(-0.26,0.97)) t=(0.00,0.00)}] [stroke={[type=SOLID]
[color=#B4B4B4] [stroke width=9.00]}] [data="M200.00,120.00 L200.00,140.00
L220.00,140.00 L220.00,160.00"]
>	 RenderPath {line} at (44.26,12.13) size 29.49x53.74
[stroke={[type=SOLID] [color=#B4B4B4] [stroke width=3.00]}] [fill={[type=SOLID]
[color=#000000]}] [data="M40.00,50.00 L20.00,10.00"]
>	 RenderPath {line} at (123.13,11.26) size 105.74x55.49
[stroke={[type=SOLID] [color=#B4B4B4] [stroke width=3.00]}] [fill={[type=SOLID]
[color=#000000]}] [data="M160.00,50.00 L80.00,10.00"]
> -	 RenderPath {line} at (59,38.35) size 117.00x1.30 [stroke={[type=SOLID]
[color=#B4B4B4]}] [fill={[type=SOLID] [color=#000000]}] [data="M30.00,30.00
L120.00,30.00"]
> +	 RenderPath {line} at (59,38.35) size 117x1.30 [stroke={[type=SOLID]
[color=#B4B4B4]}] [fill={[type=SOLID] [color=#000000]}] [data="M30.00,30.00
L120.00,30.00"]

Why are we seeing decimal numbers here when we didn't before?

> Index:
LayoutTests/platform/mac/svg/batik/text/textGlyphOrientationHorizontal-expected
.txt
> ===================================================================
> ---
LayoutTests/platform/mac/svg/batik/text/textGlyphOrientationHorizontal-expected
.txt	   (revision 40698)
> +++
LayoutTests/platform/mac/svg/batik/text/textGlyphOrientationHorizontal-expected
.txt	   (working copy)
> @@ -29,8 +29,8 @@ layer at (0,0) size 450x500
>		   chunk 1 text run 3 at (85.93,25.89) startOffset 0 endOffset
5 width 65.00: " Good"
>	     RenderSVGInlineText {#text} at (0,0) size 0x0
>	 RenderPath {line} at (50,129) size 350x2 [stroke={[type=SOLID]
[color=#0000FF] [stroke width=2.00]}] [fill={[type=SOLID] [color=#000000]}]
[data="M50.00,130.00 L400.00,130.00"]
> -	 RenderSVGText {text} at (58,125) size 351x20 contains 1 chunk(s)
> -	   RenderSVGInlineText {#text} at (-11,-15) size 351x20
> +	 RenderSVGText {text} at (58,125) size 350x20 contains 1 chunk(s)
> +	   RenderSVGInlineText {#text} at (-10,-15) size 350x20
>	     chunk 1 text run 1 at (58.00,125.00) startOffset 0 endOffset 13
width 142.00: "Batik is Good"
>	 RenderSVGContainer {g} at (40,146) size 330x55.52
[transform={m=((1.00,0.00)(0.00,1.00)) t=(30.00,150.00)}]
>	   RenderSVGContainer {use} at (49.55,158.48) size 320.45x43.05
> @@ -48,8 +48,8 @@ layer at (0,0) size 450x500
>		   chunk 1 text run 3 at (237.33,22.64) startOffset 0 endOffset
5 width 65.00: " Good"
>	     RenderSVGInlineText {#text} at (0,0) size 0x0
>	 RenderPath {line} at (50,239) size 150x2 [stroke={[type=SOLID]
[color=#0000FF] [stroke width=2.00]}] [fill={[type=SOLID] [color=#000000]}]
[data="M50.00,240.00 L200.00,240.00"]
> -	 RenderSVGText {text} at (58,240) size 143x29 contains 1 chunk(s)
> -	   RenderSVGInlineText {#text} at (-9,-23) size 143x29
> +	 RenderSVGText {text} at (58,240) size 142x27 contains 1 chunk(s)
> +	   RenderSVGInlineText {#text} at (-8,-22) size 142x27
>	     chunk 1 text run 1 at (58.00,240.00) startOffset 0 endOffset 13
width 142.00: "Batik is Good"
>	 RenderSVGContainer {g} at (224,204) size 176x70
[transform={m=((1.00,0.00)(0.00,1.00)) t=(220.00,220.00)}]
>	   RenderSVGContainer {use} at (239.29,228.42) size 160.71x43.16

I don't think these RenderSVGText size changes are caused by your changes. You
should not change these expecteds.
This also applies to lots of other updated results.
> Index: LayoutTests/svg/custom/getTransformToElement.svg
> ===================================================================
> --- LayoutTests/svg/custom/getTransformToElement.svg	(revision 40698)
> +++ LayoutTests/svg/custom/getTransformToElement.svg	(working copy)
> @@ -14,7 +14,7 @@
>	       ctm =
referenceElement.getTransformToElement(document.getElementById("redRect"));
>	       if (ctm.a.toFixed(3) == 0.354 && ctm.b.toFixed(3) == -0.354 &&
>		   ctm.c.toFixed(3) == 0.354 && ctm.d.toFixed(3) == 0.354 &&
> -		    ctm.e.toFixed(3) == -107.071 && ctm.f.toFixed(15) == 0.0) {

> +		    ctm.e.toFixed(3) == -107.071 && ctm.f.toFixed(8) == 0.0) {
>		  try {
>		    var ctm =
referenceElement.getTransformToElement(document.getElementById("group0"));
>		  } catch(e) {

Inadvertent change?


More information about the webkit-reviews mailing list