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

[Alexey Ivanov]

On Tue, 9 May 2023 06:44:20 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:
> 
>   hashCode fix

Can all the subclasses of `CssValue` be compared with correct result?

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

> 2221:         @Override
> 2222:         public boolean equals(Object val) {
> 2223:             return val instanceof CSS.FontSize size && value == size.value;

What about different units? `font-size: 1em` shouldn't be equal to `font-size: 1ex`, should it?

What about keywords? Currently, declarations `font-size: smaller` and `font-size: larger` are considered equal.

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

> 2332:         public boolean equals(Object val) {
> 2333:             return val instanceof CSS.FontFamily font
> 2334:                        && Objects.equals(family, font.family);

Could you be consistent?

The indentation at [lines 2035-2036](https://github.com/openjdk/jdk/pull/13405/files/2bc265cdb1ed0d070b21233ff9982758bef6da71..717d1a1d4e8a3eb0cea848a7e8800210f30660fd#diff-e2c9b23b1844fa877fb1c4f048a8a8b85cd21d146f963837e039ab20b9560fe7R2036) above is different.

Personally, I would align `&&` with `v` of `val`; aligning to start of `instanceof` look good too, I prefer it compared to 8-column indent you used above.

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

> 2678:             if (percentage) {
> 2679:                 return false;
> 2680:             } else {

Why are percentage values never equal?

I expect two attribute sets produced from `"margin-top: 100%"` to be equal. However, the real length on rendering could be different if the attribute is applied to different elements.

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

> 2679:                 return false;
> 2680:             } else {
> 2681:                 return val instanceof CSS.LengthValue lu && span == lu.span;

What about different units? "1mm" should not be equal to "1cm", should it?

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

Changes requested by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13405#pullrequestreview-1434591382
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1199147541
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1199063559
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1199144022
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1199144367