[Webkit-unassigned] [Bug 71811] Eliminate CSSMutableValue

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=71811


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114268|review-                     |review+
               Flag|                            |




--- Comment #10 from Nikolas Zimmermann <zimmermann at kde.org>  2011-11-09 08:09:59 PST ---
(From update of attachment 114268)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.