WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Initial patch
(9.67 KB, patch)
2008-07-19 16:17 PDT
,
Nikolas Zimmermann
oliver
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug