[Webkit-unassigned] [Bug 71811] Eliminate CSSMutableValue

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114145|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2011-11-08 14:02:19 PST ---
(From update of attachment 114145)
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".)

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