RFR: JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color [v6]

[Alexey Ivanov]

On Sun, 18 Sep 2022 15:21:29 GMT, ScientificWare <duke at openjdk.org> wrote:

>> This is referenced in Java Bug Database as
>> - [JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color](https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8293776)
>> 
>> This is tracked in JBS as 
>> - [JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color](https://bugs.openjdk.java.net/browse/JDK-8293776)
>> 
>> Adds the 4 and 8 digits color hex notations to CSS.java, as described in :
>> CSS Color Module Level 4
>> W3C Candidate Recommendation Snapshot, 5 July 2022
>> [6.2 The RGB Hexadecimal Notations: `#RRGGBB`](https://www.w3.org/TR/css-color-4/#hex-notation)
>> 
>> Designed from : [ScientificWare JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color](https://github.com/scientificware/jdk/issues/13)
>
> ScientificWare has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Corrects typos and digit extraction.
>   
>   Corrects typos.
>   And applies a better digit extraction as suggested in main-line review thread.

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 1366:

> 1364:             digits = digits.substring(1, Math.min(n, 9));
> 1365:             n--;
> 1366:         }

Should the method return `null` immediately if `n < 3 || n > 8` after the if block at lines 1363-1366? It's already known the format is invalid.

I also propose adding `hex.matcher(digits).matches()` here to avoid running the matcher at each attempt.

In short, the next statement should be:


if (n < 3 || n == 7 || n > 8 || !hex.matcher(digits).matches()) {
    return null;
}


The following code deals with a valid color notation.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 1399:

> 1397:             final char b = digits.charAt(2);
> 1398:             final char a = digits.charAt(3);
> 1399:             digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);

Does it make sense to combine these two cases into one?

Suggestion:

        if (n == 3 || n == 4) {
            final char r = digits.charAt(0);
            final char g = digits.charAt(1);
            final char b = digits.charAt(2);
            final char a = n == 4 ? digits.charAt(3) : 'f';
            digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);


The only difference between these two case is in `a` which is hard-coded as `ff` in the format string for `n == 3`.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 1406:

> 1404:         }
> 1405:         try {
> 1406:             int l = Integer.parseUnsignedInt(digits, 16);

It's not recommended to use `l` as an identifier because it could be confused with `1`. I propose `x` (hex), `v` (value), or even `i` (int). If you rename `n` to `len`, then `n` will be another option.

test/jdk/javax/swing/text/html/CSS/Hex3468DigitsColor.java line 9:

> 7:  * published by the Free Software Foundation.  Oracle designates this
> 8:  * particular file as subject to the "Classpath" exception as provided
> 9:  * by Oracle in the LICENSE file that accompanied this code.

Tests must not include *the "Classpath" exception*.

test/jdk/javax/swing/text/html/CSS/Hex3468DigitsColor.java line 71:

> 69:         alpha = color.getAlpha();
> 70:         result.append("\n  Test for #ff1122aa");
> 71:         if (red != 255) {

`red`, `green` and `blue` haven't changed here. You may want to get them from the `color` object.

Alternatively, you can compare the RGB:

if (0xaaff1122 != color.getRGB()) {
    // fail the test
}

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

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