Bug 11797 – replace SVGMatrix by AffineTransform

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

SVGMatrix is not needed, it is just the wrappers that need to add this API. Use AffineTransform internally.

[Rob Buis]

Created attachment 11799 [details]
First attempt

This patch causes no regressions, but reapplying and regenerating it gives compile problems...
Cheers,

Rob.

[Rob Buis]

Created attachment 11800 [details]
This one compiles

This one compiles, thnx WildFox! Waiting for the automatic POD generation before reviewing is needed though.
Cheers,

Rob.

[Nikolas Zimmermann]

Created attachment 11811 [details]
Updated patch

Patch updated against POD generation.

[Nikolas Zimmermann]

Created attachment 11812 [details]
Updated patch II

Incorporated some of weinig's comments.

[Sam Weinig]

Comment on attachment 11812 [details]
Updated patch II


-- CodeGeneratorObjC.pm

Move these up into CodeGenerator.pm

+my %podTypeHash = ("RGBColor" => 1, "SVGPoint" => 1, "SVGRect" => 1, "SVGNumber" => 1, "SVGMatrix" => 1);

+sub IsPodType
+{
+    my $type = shift;
+
+    return 1 if $podTypeHash{$type};
+    return 0;
+}


Please use more descriptive Variable names here and put a comment/FIXME stating that the [Custom] support is needed to fix it.  There also seem to be quite a few unneeded spaces after the first line.

+            my $exceptionOne = ($podType and $podType eq "AffineTransform" and $functionName eq "rotateFromVector");
+            my $exceptionTwo = ($podType and $podType eq "AffineTransform" and $functionName eq "inverse");


-- SVGAnimateTransformElement.cpp

Either remove the commented out code or at least put a comment in about why it is commented out.
+/*
+    if (m_transformMatrix)
         m_transformMatrix = new SVGMatrix();
     else {
-        m_transformMatrix->reset();
+        m_transformMatrix.reset();
 
         if (isAccumulated() && repeations() != 0.0 && m_lastMatrix)
             m_transformMatrix->multiply(m_lastMatrix.get());
     }
-    
+ */   


Are style, although undocumented, is to use 1.0 instead of just 1.
-            m_transformMatrix->skewX(sx);
+            m_transformMatrix.shear(sx, 1.);
and
-            m_transformMatrix->skewY(sy);
+            m_transformMatrix.shear(1., sy);


Is there a more efficient way to do this?
-                m_transformMatrix = new SVGMatrix();
+                m_transformMatrix = AffineTransform();


-- SVGPreserveAspectRatio.cpp

Nitpick, re-align function arguments

-SVGMatrix* SVGPreserveAspectRatio::getCTM(float logicX, float logicY,
+AffineTransform SVGPreserveAspectRatio::getCTM(float logicX, float logicY,
                                           float logicWidth, float logicHeight,
                                           float /*physX*/, float /*physY*/,
                                           float physWidth, float physHeight)

-- SVGTransform.cpp

Another nitpick, we tend to use the lowercase 'f' for float (I don't know that it makes a difference though)
+    m_matrix.shear(tan(SVGAngle::torad(angle)), 0.0F);
and
+    m_matrix.shear(0.0F, tan(SVGAngle::torad(angle)));


-- AffineTransformCG.cpp

Why not just set the variable directly in the struct, instead of creating a new one.  Same goes for other platforms.
+void AffineTransform::setA(double a)
+{
+    m_transform = CGAffineTransformMake(a, b(), c(), d(), e(), f());
+}


-- SVGRenderTreeAsText.cpp

Curly braces go on the same line as the else
     else 
     {
-        ts << "{m=((" << m.m11() << "," << m.m12() << ")(" << m.m21() << "," << m.m22() << "))";
-        ts << " t=(" << m.dx() << "," << m.dy() << ")}";
+        ts << "{m=((" << m.a() << "," << m.b() << ")(" << m.c() << "," << m.d() << "))";
+        ts << " t=(" << m.e() << "," << m.f() << ")}";
     }

[Nikolas Zimmermann]

> Move these up into CodeGenerator.pm

Fixed.
 
> Please use more descriptive Variable names here and put a comment/FIXME stating
> that the [Custom] support is needed to fix it.  There also seem to be quite a
> few unneeded spaces after the first line.


Fixed.

> -- SVGAnimateTransformElement.cpp
> 
> Either remove the commented out code or at least put a comment in about why it
> is commented out.

Fixed.
 
> Are style, although undocumented, is to use 1.0 instead of just 1.
> -            m_transformMatrix->skewX(sx);
> +            m_transformMatrix.shear(sx, 1.);
> and
> -            m_transformMatrix->skewY(sy);
> +            m_transformMatrix.shear(1., sy);

Fixed.

> Is there a more efficient way to do this?
> -                m_transformMatrix = new SVGMatrix();
> +                m_transformMatrix = AffineTransform();

Yes, .reset(), fixed.

> -- SVGPreserveAspectRatio.cpp
> 
> Nitpick, re-align function arguments

Fixed.
 
> -- SVGTransform.cpp
> 
> Another nitpick, we tend to use the lowercase 'f' for float (I don't know that
> it makes a difference though)
> +    m_matrix.shear(tan(SVGAngle::torad(angle)), 0.0F);
> and
> +    m_matrix.shear(0.0F, tan(SVGAngle::torad(angle)));

Fixed.

> -- AffineTransformCG.cpp
> 
> Why not just set the variable directly in the struct, instead of creating a new
> one.  Same goes for other platforms.

I've done that for all platforms except Qt, where that is not possible.

> 
> -- SVGRenderTreeAsText.cpp
> 
> Curly braces go on the same line as the else

Fixed.

Attaching new patch soon...

[Nikolas Zimmermann]

Created attachment 11816 [details]
Final patch

Adressed all of Sam's comments.

[Nikolas Zimmermann]

Created attachment 11817 [details]
Updated final patch

Mention the bug report also in LayoutTests/ChangeLog, as suggested by Rob and remove a typo.

[Nikolas Zimmermann]

Landed in r18177.