RFR: 8345348: CSS media feature queries [v13]
Michael Strauß
mstrauss at openjdk.org
Mon Apr 14 14:39:27 UTC 2025
On Mon, 14 Apr 2025 11:18:29 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
>>
>> improved implementation of NullCoalescingPropertyBase
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/TokenStream.java line 31:
>
>> 29: import java.util.function.Predicate;
>> 30:
>> 31: public final class TokenStream {
>
> I think the naming of the methods in this class leaves something to be desired.
>
> - `Token consume()` **OK**
> - `Token consume(Predicate)` -> `consumeIf` or `consumeIfMatches`
> Makes it clearer, as `consume` seems to imply something is always consumed (ie. it still skips one token if the predicate didn't match returning `null`).
> - `Token peek()` **OK**
> - `reconsume` -> `unconsume`, `undoConsume`, `undo`, `previous`, `resetToPrevious`, `decrementIndex`
> Nothing is consumed which reconsume seems to imply, instead it moves the index back one place so the next call to `consume` may indeed reconsume a token; reconsume as-is would do nothing (and if it returned anything it would be same the as `current`.
> - `boolean peekMany(Predicate...)` -> `matches`
> It doesn't work like `peek`. `peekMany` would imply it returns a `List<Token>`; it also doesn't convey that it returns a `boolean`.
> - `reset(int)` -> `setIndex`
> This seems to be similar to what say `InputStream` provides, but `InputStream` hides the `index` parameter so you can't set it to some arbitrary value (like skipping ahead). If you want to mimic this pattern, I'd suggest removing the parameter and providing a `mark` method (or make it non-public).
I renamed `consume(Predicate)` to `consumeIf(Predicate)`, and `peekMany(Predicate...)` to `matches(Predicate...)`.
`index()` and `reset()` don't need to be public, so I've removed that (also I don't want to mimic the problematic mark/reset pattern of `InputStream`).
`reconsume` is [CSS lingo](https://www.w3.org/TR/css-syntax-3/#tokenizer-definitions), I'm keeping that as-is.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042287057
More information about the openjfx-dev
mailing list