Bug 71811 – Eliminate CSSMutableValue

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

CSSMutableValue is used for implementing some deprecated SVG setters only.

[Antti Koivisto]

Created attachment 114145 [details]
patch

[Darin Adler]

Comment on attachment 114145 [details]
patch

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

I’m saying review- because I think this removes two tests that should not be removed. Patch seems otherwise great, so I would have said review+.

> Source/WebCore/svg/SVGColor.cpp:52
> +    // Deprecated.

This comment seems good, but may be a bit too terse. Seems the only value of having the setter is to control which kind of exception we generate. Maybe it would be better to remove the setter entirely. I wonder if there is any real-world benefit to keeping this stub instead of just removing it from the IDL file.

> LayoutTests/ChangeLog:10
> +        - Remove (mostlye render tree dump based) tests for these features that have little useful

Typo: mostlye

> LayoutTests/svg/custom/rgbcolor-syntax.svg:-34
> -    function testIncorrectColor() {
> -        var col = document.styleSheets[0].cssRules[0].style.getPropertyCSSValue("fill");
> -        if (!expectsException(col, "rgb(100%,100%,0%")) return;
> -        if (!expectsException(col, "rgba(100%,100%,0%")) return;
> -        if (!expectsException(col, "rgb(100%,100%,r)")) return;

Seems wrong to remove this test. Instead we should rewrite it. There must still be some way to specify RGB colors even if it’s not through the setRGBColor function.

> LayoutTests/svg/dom/rgb-color-parser-expected.txt:-6
>  This test fuzzes the length list parser 
>  On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>  
>  
> -Threw exception Error: SVG_INVALID_VALUE_ERR: DOM SVG Exception 1: rgb(.78-2e8e

This does not seem like a good change.

The point of this test is to test the color parsing. Instead we should change the test to work differently, but still test the color parsing. Because we’re not removing this color parsing code, so we should not be removing the test coverage of it.

(Also should fix "length list parser" to say "RGB color parser".)

[Antti Koivisto]

(In reply to comment #2)
> This comment seems good, but may be a bit too terse. Seems the only value of having the setter is to control which kind of exception we generate. Maybe it would be better to remove the setter entirely. I wonder if there is any real-world benefit to keeping this stub instead of just removing it from the IDL file.

The whole SVGColor and SVGPaint classes are deprecated in the SVG spec. I don't think we should start removing stuff from IDL partially. The next step would be to eliminate the types entirely.

> > LayoutTests/svg/custom/rgbcolor-syntax.svg:-34
> > -    function testIncorrectColor() {
> > -        var col = document.styleSheets[0].cssRules[0].style.getPropertyCSSValue("fill");
> > -        if (!expectsException(col, "rgb(100%,100%,0%")) return;
> > -        if (!expectsException(col, "rgba(100%,100%,0%")) return;
> > -        if (!expectsException(col, "rgb(100%,100%,r)")) return;
> 
> Seems wrong to remove this test. Instead we should rewrite it. There must still be some way to specify RGB colors even if it’s not through the setRGBColor function.

We simply call into CSS color parser. I think we have a pretty good test coverage for basic cases like these here. I'm not sure this one provides much value since it is no longer needed to test the correct setRGBColor exceptions.

> > LayoutTests/svg/dom/rgb-color-parser-expected.txt:-6
> >  This test fuzzes the length list parser 
> >  On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> >  
> >  
> > -Threw exception Error: SVG_INVALID_VALUE_ERR: DOM SVG Exception 1: rgb(.78-2e8e
> 
> This does not seem like a good change.
> 
> The point of this test is to test the color parsing. Instead we should change the test to work differently, but still test the color parsing. Because we’re not removing this color parsing code, so we should not be removing the test coverage of it.

We used to have a separate code path for parsing SVG color values and this test was added to fuzz that specifically. We now use CSS color parsing which has lots of test coverage but I'm not sure how much we fuzz it elsewhere. You are right that I should try to keep the test functional.

[Antti Koivisto]

Created attachment 114246 [details]
patch2

- Made svg/dom/rgb-color-parser.html work properly.
- Added the cases previously covered by svg/custom/rgbcolor-syntax.svg to rgb-color-parser.html. It uses a better mechanism to test the same thing.

[WebKit Review Bot]

Comment on attachment 114246 [details]
patch2

Attachment 114246 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10378390

New failing tests:
fast/css/parse-color-int-or-percent-crash.html
svg/dom/getPresentationAttribute-cache-corruption.svg

[Antti Koivisto]

Created attachment 114268 [details]
patch3

- Changed fast/css/parse-color-int-or-percent-crash.html to not use SVGColor mutations
- Removed svg/dom/getPresentationAttribute-cache-corruption.svg as it was not really testing anything else than mutations.

[Andreas Kling]

Comment on attachment 114268 [details]
patch3

The change to parse-color-int-or-percent-crash.html looks good to me.
I agree that the color parsing routines are already covered well enough that we're not losing anything of value here.
You missed changing the test to say "RGB color parser" instead of "length list parser" though.

r+ based on Darin's original comments, and my love for this patch.

[Nikolas Zimmermann]

Comment on attachment 114268 [details]
patch3

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.

[Antti Koivisto]

(In reply to comment #8)
Nikolas, could you go test by test and explain what you want to be done with them (preferrably with new test content). I find these tests very confusing.

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

We have plenty of expected FAILS in WebKit but ok.

[Nikolas Zimmermann]

Comment on attachment 114268 [details]
patch3

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

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

Ok, per Anttis request I looked at them extensively.
SVGPaint-mutate-attribute.svg can die. getPA('fill') can't have any side effects anymore and thus this test is not needed anymore.

>> LayoutTests/svg/custom/SVGPaint-mutate-inline-style.svg:-2
>> -<!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.

Same here, SVGPaint-mutate-inline-style.svg can go, we can document in a single place that objects returned by getPropertyCSSValue/getPresentationAttribute aren't mutable.

>> LayoutTests/svg/custom/getPresentationAttribute-modify.svg:-3
>> -<svg xmlns="http://www.w3.org/2000/svg" onload="runTest()">
> 
> I'd rather modify the tests to demonstrate that getPA does NOT modify anymore.

This is wrong as well, just go ahead and remove it.

>> LayoutTests/svg/custom/getPresentationAttribute.svg:27
>> +        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.

I missed the checks for border-top/color, it's still worthy. Let it as-is.

>> LayoutTests/svg/dom/SVGColor-expected.txt:38
>> +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.

Here I prefer for consistency with all other SVG tests, that we only report FAIL if we intend to ever fix this to PASS.
If we don't want to fix it, document the new behaviour and check that all methods of SVGColor/SVGPaint that mutate are expected to throw, with a comment in the testcase that this has changed, with a link to this br.

To summarize: Please change all FAILs to PASS, then I'm fine with the rest of the patch. r=me.

[Antti Koivisto]

http://trac.webkit.org/changeset/99749