Bug 12171 – Remove "points" hack from SVGPolyElement

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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;
}

[Eric Seidel (no email)]

I'm told the "style" attribute has a system in place to do this automatic regeneration as well.

[Rob Buis]

This still needs to be fixed, but low prio.

[Rob Buis]

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 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]

Easy implementable for me after 20051 landed.

[Nikolas Zimmermann]

Created attachment 22383 [details]
Initial patch

[Oliver Hunt]

Comment on attachment 22383 [details]
Initial patch

r=me, i'd like a bug filed on slow SVGPointList::valueAsString tough

[Nikolas Zimmermann]

Landed in r35254. Bug 20120 filed for slow impl. of valueAsString().