Bug 37071 – SVG/SMIL parse failure on attribute keySplines

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

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.

[Dirk Schulze]

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.

[Darin Adler]

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.

[Dirk Schulze]

(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.

[Oliver Hunt]

Comment on attachment 52522 [details]
Patch

r=me

[Dirk Schulze]

Comment on attachment 52522 [details]
Patch

Clearing flags on attachment: 52522

Committed r57140: <http://trac.webkit.org/changeset/57140>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.