[webkit-reviews] review denied: [Bug 71811] Eliminate CSSMutableValue : [Attachment 114268] patch3

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 9 07:21:53 PST 2011


Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 71811: Eliminate CSSMutableValue
https://bugs.webkit.org/show_bug.cgi?id=71811

Attachment 114268: patch3
https://bugs.webkit.org/attachment.cgi?id=114268&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114268&action=review


r- for various tests issues:

> LayoutTests/svg/custom/SVGPaint-mutate-attribute.svg:-1
> -<?xml version="1.0" encoding="ISO-8859-1" standalone="no"?>

This test shouldn't die - along the others, see below.

> LayoutTests/svg/custom/SVGPaint-mutate-inline-style.svg:-2
> -<?xml version="1.0" encoding="ISO-8859-1" standalone="no"?>
> -<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 20010904//EN"
"http://www.w3.org/TR/2001/REC-SVG-20010904/DTD/svg10.dtd">

This should test that SVGPaint mutations don't work for inline styles anymore.

> LayoutTests/svg/custom/getPresentationAttribute-modify.svg:-3
> -<?xml version="1.0" encoding="ISO-8859-1" standalone="no"?>
> -<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 20010904//EN"
"http://www.w3.org/TR/2001/REC-SVG-20010904/DTD/svg10.dtd">
> -<svg xmlns="http://www.w3.org/2000/svg" onload="runTest()">

I'd rather modify the tests to demonstrate that getPA does NOT modify anymore.

> LayoutTests/svg/custom/getPresentationAttribute.svg:27
> -	   var fill = rect.getPresentationAttribute('fill');
> -	   fill.setRGBColor("green");
> +	   rect.setAttribute("fill", "green");

This makes the test useless, it's supposed to check that
rect.getPresentationAttribute('fill') returns the right value.
You should make sure that fill is a SVGPaint, and check its color is not-green.
Then call rect.setAttribute("fill",, "green", refetch the SVGPaint using getPA,
and check its now green.

> LayoutTests/svg/dom/SVGColor-expected.txt:38
> -PASS stopColor.setColor(SVGColor.SVG_COLORTYPE_RGBCOLOR_ICCCOLOR,
'rgb(77,0,77)', 'icc-color(myRGB, 0, 1, 2)') is undefined.
> -PASS stopColor.colorType is SVGColor.SVG_COLORTYPE_RGBCOLOR_ICCCOLOR
> -PASS stopColor.colorType is SVGColor.SVG_COLORTYPE_RGBCOLOR_ICCCOLOR
> +FAIL stopColor.setColor(SVGColor.SVG_COLORTYPE_RGBCOLOR_ICCCOLOR,
'rgb(77,0,77)', 'icc-color(myRGB, 0, 1, 2)') should be undefined. Threw
exception Error: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7

You should update these tests, that they still say PASS. Otherwhise this will
lead to confusion. aka. tests that all of them throw now.

> LayoutTests/svg/dom/SVGPaint-expected.txt:47
> +FAIL fillPaint.setPaint(SVGPaint.SVG_PAINTTYPE_NONE, '', '', '') should be
undefined. Threw exception Error: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7
> +FAIL fillPaint.paintType should be 101. Was 1.

Also not good, this should be changed to report PASSes.


More information about the webkit-reviews mailing list