[Webkit-unassigned] [Bug 71811] Eliminate CSSMutableValue

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Antti Koivisto <koivisto at iki.fi>  2011-11-08 15:42:21 PST ---
(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.

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