Bug 12439 - SVG parser complains about points attribute in polygon and polyline element
Summary: SVG parser complains about points attribute in polygon and polyline element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://www.carto.net/papers/svg/sampl...
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-27 04:31 PST by Andreas Neumann
Modified: 2007-02-20 07:59 PST (History)
0 users

See Also:


Attachments
First attempt (1.54 KB, patch)
2007-01-28 04:37 PST, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Now with testcase (35.94 KB, patch)
2007-02-18 04:32 PST, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Neumann 2007-01-27 04:31:34 PST
In the given example there is a polygon and polyline element. Webkit complains in the Javascript Console:

Error: Problem parsing points="160,200 180,230 200,210 234,220"
Error: Problem parsing points="49,272 57,297 114,282 63,314 71,339 49,324 27,339 35,314 13,297 40,297"


However, as far as I know this is correct syntax and Webkit also seems to render it correctly. Maybe this is an unnecessary warning?

Maybe there is also a connection to this bug: http://bugs.webkit.org/show_bug.cgi?id=12171 ?
Comment 1 Rob Buis 2007-01-28 04:37:36 PST
Created attachment 12722 [details]
First attempt

It turns out the previous implementation never returned true, this one does, so now only errors will be reported for illegal points values.
Cheers,

Rob.
Comment 2 Darin Adler 2007-01-28 18:10:48 PST
Comment on attachment 12722 [details]
First attempt

Fix looks fine. I'd like to see the test in the patch too.

In addition, I'd like the test to be a systematic thorough test of the parser.

To see what I mean, look, for example, at the mini-test-suite I added for the preserve aspect ratio parser (LayoutTests/svg/dom/preserve-aspect-ratio-parser.html).
Comment 3 Rob Buis 2007-02-18 04:32:30 PST
Created attachment 13223 [details]
Now with testcase

Now with testcase :) Note that the patch changes behaviour on error; when there is an error parsing the points attr, the points list is cleared, this matches Opera behaviour. I also had to change a test because of this.
Cheers,

Rob.
Comment 4 Darin Adler 2007-02-19 15:03:32 PST
Comment on attachment 13223 [details]
Now with testcase

Looks great! I'd like to see a few more test cases in points-parser.html, for example with leading or trailing whitespace, but this looks really great to me.

r=me
Comment 5 Rob Buis 2007-02-20 07:59:08 PST
Landed in r19732.