On Tue, 6 May 2025 06:47:46 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/expression/FunctionExpression.java
>> line 66:
>>
>>> 64: @Override
>>> 65: public int hashCode() {
>>> 66: return Objects.hash(featureName, featureValue, value);
>>
>> very minor: maybe add `FunctionExpression.class` to the mix?
>
> Why?
so an array with the same values produces a different hash code.
in reality, it's highly unlikely.
However, `FunctionExpression` is a `record`. What's the point of declaring it
a record when you override `hashCode` and `equals`?
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/CssLexer.java
>> line 34:
>>
>>> 32:
>>> 33: public final class CssLexer {
>>> 34: public final static int STRING = 10;
>>
>> minor: `public static final` maybe?
>
> I want to keep the diffs minimal here, because I didn't change anything
> except making the constants public.
the github diff shows it as completely different block anyway, so without
copypasting to BeyondCompary I could not see it.
Might as well change to canonical ordering, no?
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/TokenStream.java
>> line 73:
>>
>>> 71: }
>>> 72:
>>> 73: public void reconsume() {
>>
>> maybe a javadoc would suffice if you like the name so much
>
> I've added javadocs to all methods. By the way, I don't really get the
> opposition to the name. The arguments presented so far do not convince me to
> drop CSS lingo.
people who are not experts with web css are confused.
anyway, I am ok with it - as long as it's documented.
thanks!
>> modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/parser/CssLexerTest.java
>> line 47:
>>
>>> 45: }
>>> 46:
>>> 47: private void checkTokens(List<Token> resultTokens, Token...
>>> expectedTokens) {
>>
>> should there be new tests in `CssLexerTest` for the `@media` queries?
>
> No, media queries are not a thing at the lexer stage. The media query grammar
> only comes in at a later stage.
oh, the handling of the `@media` tokens is done at a different level?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075748289
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075750493
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075695509
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2075755099
