RFR: 8345348: CSS media feature queries [v21]
Michael Strauß
mstrauss at openjdk.org
Tue May 6 06:52:38 UTC 2025
On Mon, 5 May 2025 15:28:30 GMT, Andy Goryachev <angorya at openjdk.org> wrote:
>> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 30 commits:
>>
>> - Merge branch 'master' into feature/media-queries
>> - cssref doc
>> - Merge branch 'master' into feature/media-queries
>> - reorder fields
>> - remove ReadOnlyBooleanWrapper
>> - Scene preferences only actively observe platform preferences when the scene is showing
>> - formatting
>> - typo
>> - use equality instead of identity
>> - rename TokenStream methods
>> - ... and 20 more: https://git.openjdk.org/jfx/compare/498b7e4c...626a904d
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaQueryParser.java line 103:
>
>> 101: default -> {
>> 102: errorHandler.accept(stream.current(), "Unexpected token");
>> 103: return expressions;
>
> is this needed? (see L108)
The return statement? Yes, we want token processing to stop here if we encounter an unexpected token, as there's no recovery strategy at this point.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaQuerySerializer.java line 104:
>
>> 102:
>> 103: public static MediaQuery readBinary(DataInputStream is, String[] strings) throws IOException {
>> 104: return switch (QueryType.VALUES[is.readByte()]) {
>
> same issue with backward compatibility - what if an old core tries to read the newer format and AIOOBE is thrown instead of IOException? Should we validate first and throw IOE instead?
That would be forward compatibility, which is not supported. JavaFX will not even try to load a binary file with a higher version than it can understand, and error out immediately (see Stylesheet.java:L317).
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaRule.java line 48:
>
>> 46: this.queries = List.copyOf(queries);
>> 47: this.parent = parent;
>> 48: this.hash = queries.hashCode();
>
> I would rather see `this.queries.hasCode()` here
I've removed it instead.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaRule.java line 118:
>
>> 116: boolean hasParent = stream.readBoolean();
>> 117:
>> 118: return new MediaRule(List.of(queries), hasParent ? readBinary(stream, strings) : null);
>
> the exceptions in java have improved, but there is 3 statements in this line.
> anyone who ever debugged code after the fact using two-week old logs would say: please keep one statement per line, if possible.
I usually don't follow dogma. That being said, I moved the parent-rule expression out of line.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaRule.java line 123:
>
>> 121: @Override
>> 122: public boolean equals(Object obj) {
>> 123: return obj instanceof MediaRule rule && rule.getQueries().equals(queries);
>
> should we add `if(obj == this)` as we usually do?
> also, `parent` is missing in the check
This is now removed.
> 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?
> 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.
> 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.
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/TokenStream.java line 103:
>
>> 101: } finally {
>> 102: currentIndex = index;
>> 103: currentItem = index >= 0 ? source.get(index) : null;
>
> general question: will this `finally` block work as expected if an exception happened somewhere in the `try` block?
That's its purpose. It will reset the stream back to the state it was in before the `matches(...)` method was called.
> modules/javafx.graphics/src/main/java/javafx/css/CssParser.java line 4339:
>
>> 4337:
>> 4338: currentToken = nextToken(lexer);
>> 4339: expectedRBraces--;
>
> what happens in case of unbalanced braces and `expectedRBraces` < 0 ?
The behavior is the same as with a normal unbalanced curly brace. I've added a test for it.
> modules/javafx.graphics/src/main/java/javafx/css/Rule.java line 384:
>
>> 382: MediaRule mediaRule = null;
>> 383:
>> 384: if (bssVersion >= 7) {
>
> please add a comment pointing to Stylesheet.BINARY_CSS_VERSION.
>
> Ideally, there would be constants declared somewhere with explanation instead of the magic number '7'
I've added comments to that effect.
> modules/javafx.graphics/src/main/java/javafx/css/Stylesheet.java line 67:
>
>> 65: * Version 5: persist @font-face
>> 66: * Version 6: converter classes moved to public package
>> 67: * Version 7: media queries
>
> this, in my opinion, deserves a constant (it does not have to be public).
I'd rather do this in a refactor PR with the rest of the magic numbers.
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 6593:
>
>> 6591: * @see #setPersistentScrollBars(Boolean)
>> 6592: */
>> 6593: boolean isPersistentScrollBars();
>
> do these accessors need individual javadocs?
Yes, because the javadoc tool doesn't recognize a method that returns `boolean` as a getter for `ObjectProperty<Boolean>` (it expects `BooleanProperty`). This is a null-coalescing property: you can set it to `null`, but it will always evaluate to `true` or `false`, so it doesn't make sense to return `Boolean` from its getter.
> modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/media/MediaQuerySerializerTest.java line 46:
>
>> 44: import static org.junit.jupiter.api.Assertions.*;
>> 45:
>> 46: public class MediaQuerySerializerTest {
>
> just a thought: this could have been a parameterized test (this version is ok though).
Yes, with the downside that parameterized tests are usually a bit harder to debug in isolation.
> 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.
> modules/javafx.graphics/src/test/java/test/javafx/css/CssParser_mediaQuery_Test.java line 185:
>
>> 183: @Test
>> 184: void disjunctionAndConjunctionCanNotBeCombined() {
>> 185: Stylesheet stylesheet = new CssParser().parse("""
>
> could we also test for "or not" and "and not" ?
I've added a test.
> modules/javafx.graphics/src/test/java/test/javafx/css/CssParser_mediaQuery_Test.java line 295:
>
>> 293:
>> 294: @Test
>> 295: void parserRecoversWhenMediaQueryIsMalformed() {
>
> should we also test malformed queries like `not not`, `and or` etc.?
>
> also, unbalanced parentheses?
I've added a test for an invalid operator combination, as well as unbalanced parentheses.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074825549
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074825763
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074826054
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074826350
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074826540
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074826650
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074826704
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074826935
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827009
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827074
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827147
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827215
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827296
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827385
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827444
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074827481
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2074825633
More information about the openjfx-dev
mailing list