RFR: 8345348: CSS media feature queries [v21]
Andy Goryachev
angorya at openjdk.org
Mon May 5 18:56:02 UTC 2025
On Sat, 3 May 2025 08:56:07 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:
>> Implementation of [CSS media queries](https://gist.github.com/mstr2/cbb93bff03e073ec0c32aac317b22de7).
>
> 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
first batch of comments. more to follow.
modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/media-query.svg line 2:
> 1: <?xml version="1.0" encoding="UTF-8" standalone="no"?>
> 2: <svg
just an observation: these images look blurry on my mac external monitor (scale=1) relative to the w3.org ones (on the left):
modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 184:
> 182:
> 183: for (DeferredProperty<?> property : deferredProperties.values()) {
> 184: property.fireValueChangedIfNecessary();
firing events from a synchronized method is a recipe for a lockup.
JavaFX is still a single threaded toolkit (if we ignore the creation of certain objects aspect), why do we want to synchronize?
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)
modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaQueryParser.java line 273:
> 271: }
> 272:
> 273: private static final Predicate<Token> IDENT = token -> token.getType() == CssLexer.IDENT;
personal preference: I'd rather see these up front.
modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaQuerySerializer.java line 72:
> 70: DataOutputStream os,
> 71: StyleConverter.StringStore stringStore) throws IOException {
> 72: os.writeByte(QueryType.of(mediaQuery).ordinal());
potential problem:
someone may insert/reorder QueryType enums and break backward compatibility.
at a minimum, a warning comment should be added to QueryType, or perhaps some more reliable code in read/writeBinary?
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?
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
modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaRule.java line 52:
> 50:
> 51: /**
> 52: * Returns the list of media queries.
should we say "immutable"?
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.
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
modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/expression/ConjunctionExpression.java line 67:
> 65: @Override
> 66: public String toString() {
> 67: return "(" + left.toString() + " and " + right.toString() + ")";
`return "(" + left + " and " + right + ")";`
modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/expression/DisjunctionExpression.java line 67:
> 65: @Override
> 66: public String toString() {
> 67: return "(" + left.toString() + " or " + right.toString() + ")";
`return "(" + left + " or " + right + ")";`
modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/expression/FunctionExpression.java line 58:
> 56: @Override
> 57: public boolean equals(Object obj) {
> 58: return obj instanceof FunctionExpression<?> expr
`if(obj == this)` ?
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?
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?
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
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?
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 ?
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'
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).
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?
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?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1655#pullrequestreview-2813024498
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073642689
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073651690
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073669724
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073756253
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073932495
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073934605
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073937065
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073937476
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073940636
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073942208
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073950949
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073952052
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073953939
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073956250
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073959700
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073965466
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073967786
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073978797
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073985918
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073989278
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073992865
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2073676691
More information about the openjfx-dev
mailing list