| Summary: | Remove "points" hack from SVGPolyElement | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | a.neumann, koivisto, oliver | ||||||
| Priority: | P4 | ||||||||
| Version: | 420+ | ||||||||
| Hardware: | Mac | ||||||||
| OS: | OS X 10.4 | ||||||||
| Bug Depends on: | 20051 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
I'm told the "style" attribute has a system in place to do this automatic regeneration as well. This still needs to be fixed, but low prio. 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.
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.
Easy implementable for me after 20051 landed. Created attachment 22383 [details]
Initial patch
Comment on attachment 22383 [details]
Initial patch
r=me, i'd like a bug filed on slow SVGPointList::valueAsString tough
|
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; }