RFR: 8345348: CSS media feature queries [v21]

[Andy Goryachev]

On Tue, 6 May 2025 06:47:46 GMT, Michael Strauß <mstrauss at 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