On Fri, 2 Jun 2023 08:12:55 GMT, Prasanta Sadhukhan <[email protected]>
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
