On Fri, 12 May 2023 23:40:03 GMT, John Hendrikx <jhendr...@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
