Re: RFR: JDK-8292276 : Missing color names in CSS. [v3]

[SWinxy]

On Mon, 15 Aug 2022 23:42:35 GMT, ScientificWare <d...@openjdk.org> wrote:

>> This is referenced in Java Bug Database as
>> - [JDK-8292276 : Missing color names in 
>> CSS](https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8292276)
>> 
>> This is tracked in JBS as 
>> - [JDK-8292276 : Missing color names in 
>> CSS](https://bugs.openjdk.java.net/browse/JDK-8292276)
>> 
>> Adds missing color names, defined by CSS Level 4, in CSS.java :
>> CSS Color Module Level 4
>> W3C Candidate Recommendation Snapshot, 5 July 2022
>> [7.1 Named Colors](https://www.w3.org/TR/css-color-4/#named-color)
>> 
>> Designed from : [ScientificWare JDK-8292276 : Missing color names in 
>> CSS](https://github.com/scientificware/jdk/issues/12)
>
> ScientificWare has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adds to CSS.java the missing color names.
>   
>   Adds to CSS.java, the missing color names defined by :
>   CSS Color Module Level 4
>   W3C Candidate Recommendation Snapshot, 5 July 2022
>   [7.1 Named Colors](https://www.w3.org/TR/css-color-4/#named-color)
>   - Updates, Copyright year.
>   - Adds relative imports.
>   - Replaces, if ... then ... else statements with a Map called "colorNamed".
>     Code is more readable and easy to maintain.
>     After tests, TreeMap seems slower than Map. Results are available at 
> scientificware#12 (comment).
>   
>   Warning : The Previous JDK CSS Orange Color is different from CSS Color 
> Recommendation.

Hey there. The current implementation creates a new `Color` object for each 
invocation of `stringToColor` (with the exception of `""` because we want to 
keep developers on their toes). Using a `Map` will *not* create new `Color` 
objects for each invocation, which may explain why your results show `Map` as 
the most performant. This is *technically* a change in behavior, and 
*technically* not wanted.

To get around this, you can do `new Color(c.getRed(), c.getGreen(), 
c.getBlue())` from the map, or--what I would prefer--to use an enhanced switch 
statement to create the colors.

One thing I'd request is to have `stringToColor` return in the branches, rather 
than setting the placeholder `Color color;` object. Things like this irk me. As 
in (using the map):

static Color stringToColor(String str) {
        if (str == null) {
            return null;
        } else if (str.length() == 0) {
            return Color.black;
        } else if (str.startsWith("rgb(")) {
            return parseRGB(str);
        } else if (str.startsWith("rgba(")) {
            return parseRGBA(str);
        } else if (str.charAt(0) == '#') {
            return hexToColor(str);
        } else if (colorNamed.containsKey(str.toLowerCase(Locale.ROOT))) {
            return colorNamed.get(str.toLowerCase(Locale.ROOT));
        } else {
            return hexToColor(str);
        }
    }


In general, good PR. I'd be interested to know the perf results when the 
behavior is unchanged, and if the enhanced switch would win out.

-------------

PR: https://git.openjdk.org/jdk/pull/9825