Summary: | Fix various problems with the SVG*List code | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||
Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, eric, mdelaney7, webkit.review.bot, zimmermann | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Nikolas Zimmermann
2010-11-21 05:17:25 PST
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.
Attachment 74500 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6212084 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 Attachment 74500 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6147110 Attachment 74500 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6157104 (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. 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 :-)
Comment on attachment 74537 [details]
Patch v2
Hopefully the bots are set to US locale :-)
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 on attachment 74537 [details] Patch v2 Clearing flags on attachment: 74537 Committed r72518: <http://trac.webkit.org/changeset/72518> All reviewed patches have been landed. Closing bug. |