RFR: 8323706: Remove SimpleSelector and CompoundSelector classes [v7]

[Andy Goryachev]

On Wed, 7 Aug 2024 02:13:12 GMT, John Hendrikx <jhendrikx at 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