Bug 37071

Summary: SVG/SMIL parse failure on attribute keySplines
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
URL: http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-89-t.svg
Attachments:
Description Flags
Patch none

Description Dirk Schulze 2010-04-04 09:35:07 PDT
WebKit fails on http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-89-t.svg . The reason is a parsing failure in attribute keySplines.
The Spec say the following about parsing (BNF):

S ::= (#x20 | #x9 | #xD | #xA)*

control-pt-set ::= ( fpval comma-wsp fpval comma-wsp fpval comma-wsp fpval )
fpval          ::= Floating point number
comma-wsp      ::= S (spacechar|",") S

'spacechar' and 'S' seems to be the same.

Furthermore, we should sperate SMIL from SVG more and move the parsing to the SMIL code. But this is a design decision and should be covered by another bug report.
Comment 1 Dirk Schulze 2010-04-05 03:24:13 PDT
Created attachment 52522 [details]
Patch

The new parsing code follows the examples in SVGParseUtilities and should be able to parse everything correctly liked discribed in the BNF of above.
Comment 2 Darin Adler 2010-04-05 08:42:23 PDT
Comment on attachment 52522 [details]
Patch

When adding a new parser like this, I'd like to see a test specifically targeting the parser.

I'm talking about tests like:

    LayoutTests/svg/dom/script-tests/path-parser.js
    LayoutTests/svg/dom/script-tests/path-segments.js

Is that kind of test case a possibility here? Without that sort of test, I am concerned we end up with insufficient coverage.
Comment 3 Dirk Schulze 2010-04-05 09:00:47 PDT
(In reply to comment #2)
> (From update of attachment 52522 [details])
> When adding a new parser like this, I'd like to see a test specifically
> targeting the parser.
> 
> I'm talking about tests like:
> 
>     LayoutTests/svg/dom/script-tests/path-parser.js
>     LayoutTests/svg/dom/script-tests/path-segments.js
> 
> Is that kind of test case a possibility here? Without that sort of test, I am
> concerned we end up with insufficient coverage.

I also thougt about a test like this. The problem is, that we don't have access to the lists. The SVG bindings don't include SMIL animations, and as far as I can see SMIL doesn't provide a way to get the lists for value, keyTime, keySplines. We can just read the whole content of an attribute. But that doesn't help.
Comment 4 Oliver Hunt 2010-04-05 16:08:41 PDT
Comment on attachment 52522 [details]
Patch

r=me
Comment 5 Dirk Schulze 2010-04-06 03:38:36 PDT
Comment on attachment 52522 [details]
Patch

Clearing flags on attachment: 52522

Committed r57140: <http://trac.webkit.org/changeset/57140>
Comment 6 Dirk Schulze 2010-04-06 03:38:45 PDT
All reviewed patches have been landed.  Closing bug.