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