On Sun, 18 Sep 2022 15:21:29 GMT, ScientificWare <d...@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
