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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted  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


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