On Wed, 7 Aug 2024 02:13:12 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Moves `SimpleSelector` and `CompoundSelector` to internal packages.
>>
>> This can be done with only a minor API break, as `SimpleSelector` and
>> `CompoundSelector` were public before. However, these classes could not be
>> constructed by 3rd parties. The only way to access them was by doing a cast
>> (generally they're accessed via `Selector` not by their sub types). The
>> reason they were public at all was because the CSS engine needs to be able
>> to access them from internal packages.
>>
>> This change fixes a mistake (or possibly something that couldn't be modelled
>> at the time) when the CSS API was first made public. The intention was
>> always to have a `Selector` interface/abstract class, with private
>> implementations (`SimpleSelector` and `CompoundSelector`).
>>
>> This PR as said has a small API break. The other changes are (AFAICS)
>> source and binary compatible:
>>
>> - Made `Selector` `sealed` only permitting `SimpleSelector` and
>> `CompoundSelector` -- as `Selector` had a package private constructor, there
>> are no concerns with pre-existing subclasses
>> - `Selector` has a few more methods that are now `protected` -- given that
>> the class is now sealed, these modified methods are not accessible (they may
>> still require rudimentary documentation I suppose)
>> - `Selector` now has a `public` default constructor -- as the class is
>> sealed, it is inaccessible
>> - `SimpleSelector` and `CompoundSelector` have a few more `public` methods,
>> but they're internal now, so it is irrelevant
>> - `createMatch` was implemented directly in `Selector` to avoid having to
>> expose package private fields in `Match` for use by `CompoundSelector`
>> - No need anymore for the `SimpleSelectorShim`
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix bug
this PR has been out for too long, could you please merge the latest master
branch in?
modules/javafx.graphics/src/main/java/com/sun/javafx/css/BinarySerializer.java
line 106:
> 104: */
> 105:
> 106: is.readByte();
should we still check the value and throw an IOE if it is wrong _for security
reasons_?
modules/javafx.graphics/src/main/java/com/sun/javafx/css/BinarySerializer.java
line 111:
> 109: }
> 110:
> 111: int nRelationships = is.readShort();
same here: should we check for a positive value?
as a general rule, we should be validating the input as it might come from
untrusted sources, right? L79 and other places?
modules/javafx.graphics/src/main/java/com/sun/javafx/css/BinarySerializer.java
line 126:
> 124: else {
> 125: assert false : "error deserializing CompoundSelector:
> Combinator = " + ordinal;
> 126: relationships.add(Combinator.DESCENDANT);
throw an exception instead?
modules/javafx.graphics/src/main/java/com/sun/javafx/css/CompoundSelector.java
line 75:
> 73: /**
> 74: * The relationships between the selectors
> 75: * @return Immutable List<Combinator>
minor: would {@code List<Combinator>} be a better choice here, if it works?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1333#pullrequestreview-2226298430
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1708023638
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1708025694
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1708026625
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1708029774
