On Sat, 3 May 2025 08:56:07 GMT, Michael Strauß <mstra...@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
