Bug 12171 - Remove "points" hack from SVGPolyElement
Summary: Remove "points" hack from SVGPolyElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P4 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 20051
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-08 16:01 PST by Eric Seidel (no email)
Modified: 2008-07-20 13:50 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 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;
}
Comment 1 Eric Seidel (no email) 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.
Comment 2 Rob Buis 2007-06-12 10:27:34 PDT
This still needs to be fixed, but low prio.
Comment 3 Rob Buis 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.
Comment 4 Eric Seidel (no email) 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.
Comment 5 Nikolas Zimmermann 2008-07-17 11:06:08 PDT
Easy implementable for me after 20051 landed.
Comment 6 Nikolas Zimmermann 2008-07-19 16:17:49 PDT
Created attachment 22383 [details]
Initial patch
Comment 7 Oliver Hunt 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
Comment 8 Nikolas Zimmermann 2008-07-20 13:50:46 PDT
Landed in r35254. Bug 20120 filed for slow impl. of valueAsString().