[Webkit-unassigned] [Bug 12171] Remove "points" hack from SVGPolyElement

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 20 14:04:50 PDT 2008


http://bugs.webkit.org/show_bug.cgi?id=12171


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #21215|review?                     |review-
               Flag|                            |




------- Comment #4 from eric at webkit.org  2008-05-20 14:04 PDT -------
(From update of attachment 21215)
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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list