RESOLVED FIXED 12171
Remove "points" hack from SVGPolyElement
https://bugs.webkit.org/show_bug.cgi?id=12171
Summary Remove "points" hack from SVGPolyElement
Eric Seidel (no email)
Reported 2007-01-08 16:01:52 PST
SVGPolyElement has a grotesque hack in order to enable getAttribute("points") working correctly. We have to be able to come up with a nicer solution: void SVGPolyElement::notifyAttributeChange() const { if (m_ignoreAttributeChanges || ownerDocument()->parsing()) return; m_ignoreAttributeChanges = true; SVGStyledElement::notifyAttributeChange(); ExceptionCode ec = 0; // Spec: Additionally, the 'points' attribute on the original element // accessed via the XML DOM (e.g., using the getAttribute() method call) // will reflect any changes made to points. String _points; int len = points()->numberOfItems(); for (int i = 0; i < len; ++i) { FloatPoint p = points()->getItem(i, ec); _points += String::format("%.6lg %.6lg ", p.x(), p.y()); } RefPtr<Attr> attr = const_cast<SVGPolyElement*>(this)->getAttributeNode(SVGNames::pointsAttr.localName()); if (attr) { ExceptionCode ec = 0; attr->setValue(_points, ec); } m_ignoreAttributeChanges = false; }
Attachments
First attempt (16.51 KB, patch)
2008-05-17 10:31 PDT, Rob Buis
eric: review-
Initial patch (9.67 KB, patch)
2008-07-19 16:17 PDT, Nikolas Zimmermann
oliver: review+
Eric Seidel (no email)
Comment 1 2007-01-08 16:05:59 PST
I'm told the "style" attribute has a system in place to do this automatic regeneration as well.
Rob Buis
Comment 2 2007-06-12 10:27:34 PDT
This still needs to be fixed, but low prio.
Rob Buis
Comment 3 2008-05-17 10:31:31 PDT
Created attachment 21215 [details] First attempt In order to remove the hack I propose what I did in the patch, ie. change Attribute::value so that you can compute the value on the fly in cases where it needs to be synced to some dom datastructure. I also applied it to path since it needs the same mechanism for path data. Cheers, Rob.
Eric Seidel (no email)
Comment 4 2008-05-20 14:04:50 PDT
Comment on attachment 21215 [details] First attempt 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.
Nikolas Zimmermann
Comment 5 2008-07-17 11:06:08 PDT
Easy implementable for me after 20051 landed.
Nikolas Zimmermann
Comment 6 2008-07-19 16:17:49 PDT
Created attachment 22383 [details] Initial patch
Oliver Hunt
Comment 7 2008-07-20 13:44:51 PDT
Comment on attachment 22383 [details] Initial patch r=me, i'd like a bug filed on slow SVGPointList::valueAsString tough
Nikolas Zimmermann
Comment 8 2008-07-20 13:50:46 PDT
Landed in r35254. Bug 20120 filed for slow impl. of valueAsString().
Note You need to log in before you can comment on or make changes to this bug.