RFR: 7083187: Class CSS.CssValue is missing implementations of equals() and hashCode() [v15]

[Alexey Ivanov]

On Fri, 2 Jun 2023 08:12:55 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Two CSS AttributeSet-s can be compared using the AttributeSet.isEqual() method which can fail due to missing implementation of equals method in CSS subclasses.
>> In this issue, even when two CSS AttributeSet has same 42 font size string value, Object equality fails.
>> Fixed by implementing the equality and hashCode method for CSS.FontSize class.
>> 
>> All jtreg/jck tests are ok
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Optimization removed to use local variables for non-percentage units

Overall, it covers most of the values now. There are a couple of omissions though: `BackgroundImage`, `BorderWidthValue` (it could be handled automatically by its superclass, `LengthValue`).

I assume `CssValue` is never used directly, it it?

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

> 2216:         @Override
> 2217:         public int hashCode() {
> 2218:             return Float.hashCode(value);

Now that `equals` method takes into account `value`, `index` and `lu`, they could be added to `hashCode`.

(On the other hand, any `CssValue` object is unlikely to be used as a key in a map.)

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

> 2479:         @Override
> 2480:         public boolean equals(Object val) {
> 2481:             return val instanceof CSS.ColorValue color && c.equals(color.c);

Is it possible that the value of `c` is null? You have the null-check in `hashCode` but you don't test it in `equals`.

`ColorValue.parseCssValue` returns `null` if it can't parse the color. So, `c` should never be `null`.

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

> 2543:         @Override
> 2544:         public boolean equals(Object val) {
> 2545:             return val instanceof CSS.BorderStyle border && style.equals(border.style);

The `style` field is of type `CSS.Value` which doesn't implement `equals` (yet). It shouldn't be a problem, however, you may use `==` explicitly.

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

> 2675:         @Override
> 2676:         public int hashCode() {
> 2677:             return Float.hashCode(span);

Should `hashCode` take into account `percentage` and `lu` fields which are used in `equals`?

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

> 2682:             if (percentage) {
> 2683:                 return val instanceof CSS.LengthValue lu
> 2684:                        && Objects.equals(svalue, lu.svalue);

Doesn't comparing `span` work for percentage values? The comment for `span` field implies it should contain the parsed value.

This comparison could fail for the case where there's a space before the `%` in the string.

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

> 2916:             hashCode |= Float.hashCode(verticalPosition);
> 2917:             hashCode |= Short.hashCode(relative);
> 2918:             return hashCode;

It's a nit yet you can simply return the computed value:
Suggestion:

            return Float.hashCode(horizontalPosition)
                   | Float.hashCode(verticalPosition)
                   | Short.hashCode(relative);

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

> 3087:         @Override
> 3088:         public int hashCode() {
> 3089:             return Float.hashCode(value);

Should it take into account other fields which are used in `equals`?

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

PR Review: https://git.openjdk.org/jdk/pull/13405#pullrequestreview-1467436669
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221444880
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221469195
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221476275
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221506112
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221504991
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221508837
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1221511728