[webkit-reviews] review denied: [Bug 11797] replace SVGMatrix by AffineTransform : [Attachment 11812] Updated patch II

[bugzilla-request-daemon at macosforge.org]

Sam Weinig <sam at webkit.org> has denied Sam Weinig <sam at webkit.org>'s request
for review:
Bug 11797: replace SVGMatrix by AffineTransform
http://bugs.webkit.org/show_bug.cgi?id=11797

Attachment 11812: Updated patch II
http://bugs.webkit.org/attachment.cgi?id=11812&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>

-- 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() << ")}";
     }