[webkit-reviews] review denied: [Bug 71811] Eliminate CSSMutableValue : [Attachment 114145] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 71811: Eliminate CSSMutableValue
https://bugs.webkit.org/show_bug.cgi?id=71811

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

------- Additional Comments from Darin Adler <darin at apple.com>
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".)