Bug 49880 - Fix various problems with the SVG*List code
: Fix various problems with the SVG*List code
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-11-21 05:17 PST by
Modified: 2010-11-22 04:33 PST (History)


Attachments
Patch (934.66 KB, patch)
2010-11-21 05:45 PST, Nikolas Zimmermann
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (886.82 KB, patch)
2010-11-22 03:13 PST, Nikolas Zimmermann
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-11-21 05:17:25 PST
* 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.
------- Comment #1 From 2010-11-21 05:45:06 PST -------
Created an attachment (id=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.
------- Comment #2 From 2010-11-21 06:14:16 PST -------
Attachment 74500 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6212084
------- Comment #3 From 2010-11-21 06:17:27 PST -------
(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?

> 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
------- Comment #4 From 2010-11-21 07:12:27 PST -------
Attachment 74500 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6147110
------- Comment #5 From 2010-11-21 07:16:29 PST -------
Attachment 74500 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6157104
------- Comment #6 From 2010-11-22 03:05:05 PST -------
(In reply to comment #3)
> (From update of attachment 74500 [details] [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.
------- Comment #7 From 2010-11-22 03:13:28 PST -------
Created an attachment (id=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 :-)
------- Comment #8 From 2010-11-22 03:51:08 PST -------
(From update of attachment 74537 [details])
Hopefully the bots are set to US locale :-)
------- Comment #9 From 2010-11-22 04:30:19 PST -------
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 keishi@webkit.org, pfeldman@chromium.org, and yurys@chromium.org.  The commit-queue is continuing to process your patch.
------- Comment #10 From 2010-11-22 04:33:00 PST -------
(From update of attachment 74537 [details])
Clearing flags on attachment: 74537

Committed r72518: <http://trac.webkit.org/changeset/72518>
------- Comment #11 From 2010-11-22 04:33:08 PST -------
All reviewed patches have been landed.  Closing bug.