On Mon, 22 Jul 2024 20:46:08 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Fixes an infinite loop that can occur while resolving lookups.
>>
>> There were 2 bugs:
>> - A `contains` check was done on some value X, but then a value Y was added
>> to the set used to track duplicates
>> - The `Set` to track duplicates was cleared in some recursive calls, meaning
>> that the caller (which may be processing multiple values in a loop) would
>> end up with an empty set, losing track of what was visited so far
>>
>> Also removed a redundant `null` check (an NPE would have occurred before it
>> could reach that code).
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add lines I forgot to commit
lgtm, with a couple of very minor comments.
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 33:
> 31: import javafx.css.CssParser;
> 32: import javafx.css.CssParser.ParseError;
> 33: import javafx.css.CssParser.ParseError.PropertySetError;
very minor: I would have used `ParseError.PropertySetError` in the code instead
of static import, removes the guesswork when looking at the code.
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 699:
> 697:
> 698: // Note: on Windows, the message is using inconsistent line
> endings (sometimes Windows, sometimes Linux)
> 699: // so I've stripped it.
would looking for substrings be a better solution? for example:
contains("Caught java.lang.IllegalArgumentException: Loop detected in *.root{")
&&
contains("while resolving '-fx-base'' while calculating value for
'-fx-background-color' from rule '*.pane' in stylesheet"
-------------
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1505#pullrequestreview-2192657068
PR Review Comment: https://git.openjdk.org/jfx/pull/1505#discussion_r1687212293
PR Review Comment: https://git.openjdk.org/jfx/pull/1505#discussion_r1687210456
