RFR: JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClassState [v7]

[Andy Goryachev]

On Fri, 12 May 2023 23:40:03 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> The class `PseudoClassState` is private API, but was exposed erroneously in the CSS API. Instead, `Set<PseudoClass>` should have been used. This PR corrects this.
>
> John Hendrikx has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Override hashCode with a comment explaining why
>  - Fix style issues
>  - Restore removed public method

two sticky points:
- verify somehow that nulls cannot be passed to *All() methods
- consider adding a component type to BitSet

modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 233:

> 231:     @Override
> 232:     public boolean containsAll(Collection<?> c) {
> 233:         if (this.getClass() != c.getClass()) {

this change is not equivalent: in the old code, null `c` would return false, in the new it'll throw an NPE

modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 266:

> 264:     @Override
> 265:     public boolean addAll(Collection<? extends T> c) {
> 266:         if (this.getClass() != c.getClass()) {

same, handling of the null 'c' argument.
I wonder if this is intended behavior?

modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 342:

> 340:     @Override
> 341:     public boolean retainAll(Collection<?> c) {
> 342:         if (this.getClass() != c.getClass()) {

ditto, null 'c'

modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 433:

> 431:     @Override
> 432:     public boolean removeAll(Collection<?> c) {
> 433:         if (this.getClass() != c.getClass()) {

null 'c'

modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line 532:

> 530:             return true;
> 531:         }
> 532:         if (obj instanceof BitSet<?> bitSet) {  // fast path if other is a BitSet

I think we may need to add a 'component type' to BitSet, to make sure than BitSet<A> never intersects with BitSet<B>.

(Didn't we do it already, may be in some other PR?)

modules/javafx.graphics/src/main/java/javafx/css/Match.java line 54:

> 52:     final int specificity;
> 53: 
> 54:     Match(final Selector selector, Set<PseudoClass> pseudoClasses, int idCount, int styleClassCount) {

just curious: do we really need a 'final' for an effectively final argument?

modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java line 135:

> 133:             node.styleHelper.cacheContainer.forceSlowpath = true;
> 134: 
> 135:             if (triggerStates[0] != null) {

I see that a null check was added.
Are we sure we added it to *every* possible place?

(it's not trivial to verify that with Eclipse without massive changes)

-------------

Changes requested by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1070#pullrequestreview-1431658165
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1197058675
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1197059303
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1197059913
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1197060203
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1197064223
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1197065543
PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1197077475