[webkit-reviews] review denied: [Bug 10750] SVG needs SVGAnimatedTypeNamePropertyName<ClassName> classes : [Attachment 10494] Updated complete patch

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 10750: SVG needs SVGAnimatedTypeNamePropertyName<ClassName> classes
http://bugzilla.opendarwin.org/show_bug.cgi?id=10750

Attachment 10494: Updated complete patch
http://bugzilla.opendarwin.org/attachment.cgi?id=10494&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
This is going to take a couple passes.	314k is a big patch to try and review
in one go.

darin is never a fan of these comments (with ellipses):
+    # Start actual generation...

This should probably be a subroutine:
-	 $implIncludes{"${type}.h"} = 1;
+	 if ($type ne "SVGRect" and
+	     $type ne "SVGPoint" and
+	     $type ne "SVGNumber") {
+	     $implIncludes{"${type}.h"} = 1;
+	 }

This also seems rather specific and probably should be a sub-routine:
-    push(@headerContent, "class $implClassName;\n\n");
-    
+    push(@headerContent, "class $implClassName;\n\n")
unless($codeGenerator->IsSVGAnimatedType($implClassName));
+

Again, subroutine:

+  if ($className =~ /^JSSVGAnimated/) {
+    my $type = $interfaceName;
+    $type =~ s/SVGAnimated//; 
+
+    if ($type eq "SVGRect" or
+	 $type eq "SVGPoint" or
+	 $type eq "SVGNumber") {
+      $implIncludes{"JSSVG$type.h"} = 1;
+    } else {
+      push(@implContentHeader, "#include \"PlatformString.h\"\n") if($type eq
"String");
+    }
+  }

+    return 1 if $type eq "SVGAnimatedAngle" or
+		 $type eq "SVGAnimatedBoolean" or
+		 $type eq "SVGAnimatedEnumeration" or
+		 $type eq "SVGAnimatedInteger" or


This would be easier written by using:
$type =~ s/SVGAnimated/
%w{ Angle Boolean Enumeration }.contains $type, or something similar.
(I think that works in perl, I know it does in ruby...)

Tabs:
+	JSSVGAnimatedAngle.h \
+	JSSVGAnimatedBoolean.h \


I don't understand what "nodptr" is.  Seems like a bad name if nothing else:
+  interface [nodptr, Conditional=SVG] SVGAnimatedPoints { 

darin prefers new Foo; in these sorts of cases (no ()):
+    , m_tableValues(new SVGNumberList())
Yet another thing which should be added to the style guidelines.

I've been removing KSVG_ as well from these:
-#endif // KSVG_SVGPreserveAspectRatioImpl_H
+#endif // KSVG_SVGPreserveAspectRatio_H

since it's now sorta all WebCore (not that we don't like KSVG!)

No need for the argument name here:
+	 SVGTransform* createSVGTransformFromMatrix(SVGMatrix* matrix) const;

What a fabulous change:
-    viewBoxBaseValue()->setX(x);
-    viewBoxBaseValue()->setY(y);
-    viewBoxBaseValue()->setWidth(w);
-    viewBoxBaseValue()->setHeight(h);
+    setViewBoxBaseValue(FloatRect(x, y, w, h));

Normally we do ! instead of == 0, also if( needs a space:
+    if(viewBoxRect.width() == 0 || viewBoxRect.height() == 0)

Conditional=SVG seems redundant if everything is inside module SVG:
+module svg
+{
+  interface [Conditional=SVG] SVGAnimatedRect { 


No need to explicitly initialize m_vector:

+	 SVGListBase() : m_vector() { }


These are beautiful:
+	// Specialization for String...
+	template<>
+    class SVGList<String> : public SVGListBase<String>

This needs to move off of DeprecatedString, but not necessarily in this patch:

+void SVGStringList::reset(const DeprecatedString& str)
 {

We need to have a good method of codifying this:
+	      attribute core::DOMString baseVal;
+			  // raises DOMException on setting

I think you already have a FIXME in SVGAnimatedTemplate.  WE shoudl file a bug
as well though.


again, confused by "nodptr"

+  interface [nodptr, Conditional=SVG] SVGAnimatedPathData { 

Seems silly to have a separate line for return here:
+    RefPtr<ClassName::SVGAnimatedTemplate##UpperProperty> ret; \
+    ret = new ClassName::SVGAnimatedTemplate##UpperProperty(context); \
+    return ret; \

here again:
+    RefPtr<ClassName::SVGAnimatedTemplate##UpperProperty> ret; \
+    ret = new ClassName::SVGAnimatedTemplate##UpperProperty(this); \
+    return ret; \

No need to create a new floatRect here on property at a time, or?
+	    
static_cast<RenderSVGContainer*>(renderer())->setViewBox(FloatRect(viewBox().x(
), viewBox().y(), viewBox().width(), viewBox().height()));

and again:
+    rootContainer->setViewBox(FloatRect(viewBox().x(), viewBox().y(),
viewBox().width(), viewBox().height()));

I'm not sure why this is necessary:
+// Special handling for WebCore::FloatRect
+template<>
+inline FloatRect SVGDocumentExtensions::baseValue<FloatRect>(const SVGElement*
element, const AtomicString& propertyName) const

You can have pointer and non-pointer specializations.  Vector uses something
like that.

This may be simply the most beautiful patch I have ever seen. :)

I would r+ it right here and now, but since you don't (yet!) have commit
privileges, I think we should go one more round on this one to fix a few of the
little issues I've mentioned above.

I *can't wait* to see this land!!