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

[Alexey Ivanov]

On Mon, 19 Sep 2022 16:36:00 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> 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.
>
> 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.

> @aivanov-jdk, I suppose that most of color notations are correct.

Likely, otherwise the colors wouldn't show up.

> Adding these tests in first place has a time cost.

The time is rather negligible, in my opinion. Yet the code is clearer.

If you care about micro-optimisations, the `n == 6` case should be the first one in the chain of `if`-blocks as the most common case.

> With `&&`, `hex.matcher(digits).matches()` is evaluated only once. That why I separated length test from hex test.

Yes, you're absolutely right. I agree it is evaluated only once, which wasn't apparent to me from the start. Someone else may also consider it as inefficiency.

> Pattern usage has a significant time cost.

Exactly. Is it even needed? The previous code did well without it.

I'm sure it's not needed, `Integer.parseUnsignedInt` validates the digits are valid, if not, it throws `NumberFormatException`. Since, as you say, it's rare that the color notation is invalid, I see no reason to avoid the exception using the pattern.

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

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