[webkit-reviews] review denied: [Bug 11167] SVG idls may be missing some constructors : [Attachment 12030] First attempt

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Tue Dec 26 08:31:05 PST 2006


Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 11167: SVG idls may be missing some constructors
http://bugs.webkit.org/show_bug.cgi?id=11167

Attachment 12030: First attempt
http://bugs.webkit.org/attachment.cgi?id=12030&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
The tests would be easier to debug if they used an expect()-like function which
printed PASS or FAIL per-test.	(It could even not print FAIL and you could
track a bool for the final PASS printing.)

Iface is an awful postfix.  "SVGZoomAndPanInterface" would be better.

I'm not sure I understand why we need SVGZoomAndPanInterface.  i guess the
bindings expect to be able to instantiate one in order to grab the constants
off of it?  That seems silly.

SVGZoomAndPan (if really needed) should be NonInheritable (I believe that
exists in WTF).

+    my $name = $dataNode->name;
+    if ($name eq "SVGZoomAndPan") {
+      @{$dataNode->attributes} = ();
+    }

Eeek.  What a hack.  I'm sure we can find a better way.

I think your editor wrapped SVGElement to another line:
+    interface [Conditional=SVG, GenerateConstructor] SVGFEColorMatrixElement
+						      : SVGElement

Overall this is a good change, but the SVGZoomAndPan stuff needs to be sorted
out before we land it.	You should chat with WildFox or andersca, they may have
ideas about better ways to do this.  Weinig may also have an opinion.



More information about the webkit-reviews mailing list