On Mon, 10 Apr 2023 12:44:08 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 The change looks good but it doesn't address the whole problem raised in the [JDK-7083187](https://bugs.openjdk.org/browse/JDK-7083187): `CSS.CssValue` does not implement `equals`. This fix addresses only one particular case: `CSS.FontSize` for `font-size` property. I do not think it resolves the problem *entirely*: `CssValue` and all its subclasses must implement `equals` method, otherwise adding another CSS attribute to `AttributeSet` will lead to this same issue described in the bug report. src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2203: > 2201: } > 2202: > 2203: public int hashCode() { You should add `@Override` annotation and to `equals` method too. src/java.desktop/share/classes/javax/swing/text/html/CSS.java line 2213: > 2211: return false; > 2212: } > 2213: return false; Suggestion: return val instanceof CSS.FontSize size && value == size.value; I won't insist though. test/jdk/javax/swing/text/html/CSS/CSSBug.java line 25: > 23: /* > 24: * @test > 25: * @key headful Does it need to be headful? test/jdk/javax/swing/text/html/CSS/CSSBug.java line 34: > 32: import javax.swing.text.html.StyleSheet; > 33: > 34: public class CSSBug { Could this be a more specific name? I believe the agreement is giving descriptive test names, right? test/jdk/javax/swing/text/html/CSS/CSSBug.java line 47: > 45: > 46: if (a.isEqual( b) ) { > 47: System.out.println( "a equals b"); Could you please the remove the extra spaces after and before the parentheses. ------------- Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13405#pullrequestreview-1381636095 PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1164340214 PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1164341958 PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1164342718 PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1164344571 PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1164345588
