On Mon, 5 May 2025 15:28:30 GMT, Andy Goryachev <ango...@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
