[webkit-reviews] review denied: [Bug 12171] Remove "points" hack from SVGPolyElement : [Attachment 21215] First attempt

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 12171: Remove "points" hack from SVGPolyElement
http://bugs.webkit.org/show_bug.cgi?id=12171

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
A few comments:

1.  Do the tests pass already?	If so, you should feel free to land them even
before you complete this patch as is.

2.  Ideally the tests would use the js-pre stuff in fast/js/resources  We try
to use those common "shouldBe" functions, etc. in all new tests.  I know that
could get kinda tricky with these being SVG tests.  I eventually had planned to
augment the JS testing system to allow js-only svg tests.  (where they would
correctly use an SVG template instead of an html template).

3.  I'm not sure it's OK to make Attribute::value virtual.  You should ask
hyatt or mjs.  I have performance concerns about such a basic function being
virtual.

+		 RefPtr<SVGPathSeg> seg = path->pathSegList()->getItem(i, ec);
+		 d += seg->toString();
+		 if (++i == len) break;

+= is a slow way to build strings.  I think we have a Vector<UChar> way to
build strings these days.  Or possibly a StringBuilder class, not sure.

Also, the if/break line needs a return before the break.

Again, slow:
+		 points += String::format("%.6lg %.6lg", p.x(), p.y());
+		 if (++i == len) break;

You could preallocate a buffer large enough for the entire string pretty
easily.

r- for the perf concerns.