On Tue, 9 May 2023 06:44:20 GMT, Prasanta Sadhukhan <psadhuk...@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
