Bug 49880 – Fix various problems with the SVG*List code

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

* SVGTests uses SVGStringList, it doesn't support SVGStringList <-> XML dom synchronization at the moment.
* For Firefox compatibility, we should raise SVG_WRONG_TYPE_ERR instead of TYPE_MISMATCH_ERR in several places.
* animVal should always be readonly for every type, not just lists.
* Fix removeItem() corner case, where the item to be removed was the last in the list, currently crashed.
* SVGTransformList doesn't create a real transform string, but always dumps matrix(a, b, c, d, e, f).
* SVGNumberList should use space as seperator, when dumping (FF/Opera compatible)
* Same for SVGLengthList.
* Enable StrictTypeChecking/RequiresAllArguments=Raise on all SVG*List classes, that were missing them.

[Nikolas Zimmermann]

Created attachment 74500 [details]
Patch

This brings us close to consider the SVG DOM primitive implementation as finished.
Didn't try V8 build, waiting for EWS results.

[WebKit Review Bot]

Attachment 74500 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6212084

[Dirk Schulze]

Comment on attachment 74500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74500&action=review

> WebCore/svg/SVGTransform.cpp:139
> +        builder.append(makeString("matrix(", String::number(m_matrix.a()), ' ', String::number(m_matrix.b()), ' ', String::number(m_matrix.c()), ' '));

Is String::number platform and language aware?

> WebCore/svg/properties/SVGListProperty.h:298
> +            ec = INDEX_SIZE_ERR;

Is thee no SVGException for this?

> LayoutTests/platform/mac-leopard/svg/batik/text/smallFonts-expected.txt:16
>          RenderSVGInlineText {#text} at (0,0) size 0x0
>            chunk 1 (middle anchor) text run 1 at (35.50,1.00) startOffset 0 endOffset 41 width -62.00: "Small font size test (viewBox=\"0 0 9 10\")"
>        RenderSVGContainer {g} at (0,50) size 400x450
> -        RenderSVGText {text} at (-26,1) size 3x1 contains 1 chunk(s)
> -          RenderSVGInlineText {#text} at (26,-1) size 0x0
> -            chunk 1 text run 1 at (0.90,2.00) startOffset 0 endOffset 16 width -26.00: "Text can change "
> +        RenderSVGText {text} at (-16,1) size 3x1 contains 1 chunk(s)
> +          RenderSVGInlineText {#text} at (16,-1) size 0x0

This test does not belong to the patch right? You just update the baseline. I'd prefer to see it in another patch. But please mention it in the ChangeLog if you want to upload it with this patch.

> LayoutTests/svg/dom/SVGLengthList-basics-expected.txt:117
> +PASS text1.x.baseVal.appendItem(text1) threw exception TypeError: Type error.
> +PASS text1.x.baseVal.appendItem(null) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.

Why do the exceptions differ?

> LayoutTests/svg/dom/SVGNumberList-basics-expected.txt:15
> +PASS text1.rotate.baseVal.initialize(text1) threw exception TypeError: Type error.
> +PASS text1.rotate.baseVal.initialize(null) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.

Still not sure why we have TypeError: Type error and Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0

[Eric Seidel (no email)]

Attachment 74500 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6147110

[Eric Seidel (no email)]

Attachment 74500 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6157104

[Nikolas Zimmermann]

(In reply to comment #3)
> (From update of attachment 74500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74500&action=review
> 
> > WebCore/svg/SVGTransform.cpp:139
> > +        builder.append(makeString("matrix(", String::number(m_matrix.a()), ' ', String::number(m_matrix.b()), ' ', String::number(m_matrix.c()), ' '));
> 
> Is String::number platform and language aware?
Unfortunately yes, because under the hood it just uses String::format.
Note, it's exactly the same behaviour as before, previously we used String::number from valueAsString() in SVGTransformList, I just moved it to SVGTransform.
Fixing that bug (String::number) is out of scope for this bug.

> 
> > WebCore/svg/properties/SVGListProperty.h:298
> > +            ec = INDEX_SIZE_ERR;
> 
> Is thee no SVGException for this?
        enum SVGExceptionCode {
            SVG_WRONG_TYPE_ERR          = SVGExceptionOffset,
            SVG_INVALID_VALUE_ERR       = SVGExceptionOffset + 1,
            SVG_MATRIX_NOT_INVERTABLE   = SVGExceptionOffset + 2
        };

What would you use? :-)
Despite it's not specified at all. See http://www.w3.org/TR/SVG11/types.html#__svg__SVGLengthList__getItem. No exception is raised, according to the spec, if you pass an invalid index, which makes zero sense. Firefox/Opera also throw.

> 
> > LayoutTests/platform/mac-leopard/svg/batik/text/smallFonts-expected.txt:16
> >          RenderSVGInlineText {#text} at (0,0) size 0x0
> >            chunk 1 (middle anchor) text run 1 at (35.50,1.00) startOffset 0 endOffset 41 width -62.00: "Small font size test (viewBox=\"0 0 9 10\")"
> >        RenderSVGContainer {g} at (0,50) size 400x450
> > -        RenderSVGText {text} at (-26,1) size 3x1 contains 1 chunk(s)
> > -          RenderSVGInlineText {#text} at (26,-1) size 0x0
> > -            chunk 1 text run 1 at (0.90,2.00) startOffset 0 endOffset 16 width -26.00: "Text can change "
> > +        RenderSVGText {text} at (-16,1) size 3x1 contains 1 chunk(s)
> > +          RenderSVGInlineText {#text} at (16,-1) size 0x0
> 
> This test does not belong to the patch right? You just update the baseline. I'd prefer to see it in another patch. But please mention it in the ChangeLog if you want to upload it with this patch.
It's a leftover, didn't intend to include it here. I'll remove it for the next patch upload.

> 
> > LayoutTests/svg/dom/SVGLengthList-basics-expected.txt:117
> > +PASS text1.x.baseVal.appendItem(text1) threw exception TypeError: Type error.
> > +PASS text1.x.baseVal.appendItem(null) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.
> 
> Why do the exceptions differ?
Easy answer: appendItem of SVGLengthList requires a SVGLength object.
That means passing an integer, a string, or a SVGTextElement as object, will result in a JS type error. As the conversion from int/string/SVGTextElement to SVGLength is not possible. That explains the first set of messasges "TypeError:...".
When passing 'null', it can be converted to a SVGPropertyTearOff<SVGLength>* object (which is used internally). We're then passing this null pointer to SVGListPropertyTearOff<SVGLengthList>::appendItem, which will then raise SVG_WRONG_TYPE_ERR.
Firefox handles it the same way:

Test uncommon arguments for appendItem()
PASS text1.x.baseVal.appendItem(30) threw exception [Exception... "Could not convert JavaScript argument arg 0 [nsIDOMSVGLengthList.appendItem]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: file:///Users/nikolaszimmermann/Coding/WebKit/LayoutTests/fast/js/resources/js-test-pre.js :: shouldThrow :: line 266"  data: no].
PASS text1.x.baseVal.appendItem('aString') threw exception [Exception... "Could not convert JavaScript argument arg 0 [nsIDOMSVGLengthList.appendItem]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: file:///Users/nikolaszimmermann/Coding/WebKit/LayoutTests/fast/js/resources/js-test-pre.js :: shouldThrow :: line 266"  data: no].
PASS text1.x.baseVal.appendItem(text1) threw exception [Exception... "Could not convert JavaScript argument arg 0 [nsIDOMSVGLengthList.appendItem]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: file:///Users/nikolaszimmermann/Coding/WebKit/LayoutTests/fast/js/resources/js-test-pre.js :: shouldThrow :: line 266"  data: no].
PASS text1.x.baseVal.appendItem(null) threw exception [Exception... "Unknown or invalid type"  code: "0" nsresult: "0x80620000 (NS_ERROR_DOM_SVG_WRONG_TYPE_ERR)"  location: "file:///Users/nikolaszimmermann/Coding/WebKit/LayoutTests/fast/js/resources/js-test-pre.js Line: 266"].


> 
> > LayoutTests/svg/dom/SVGNumberList-basics-expected.txt:15
> > +PASS text1.rotate.baseVal.initialize(text1) threw exception TypeError: Type error.
> > +PASS text1.rotate.baseVal.initialize(null) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.
> 
> Still not sure why we have TypeError: Type error and Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0
See above.

[Nikolas Zimmermann]

Created attachment 74537 [details]
Patch v2

New version, fixed v8 build, and removed batik/text/smallFonts.svg rebaseline.
Also setting cq? as I'm not here the whole day, if it builds everywhere, and there are no more comments, I'd like cq+ as well :-)

[Dirk Schulze]

Comment on attachment 74537 [details]
Patch v2

Hopefully the bots are set to US locale :-)

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74537 [details]:

inspector/syntax-highlight-css.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74537 [details]
Patch v2

Clearing flags on attachment: 74537

Committed r72518: <http://trac.webkit.org/changeset/72518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.