Skip to content

Commit 3fa02ee

Browse files
hjohnhjohn
committedMay 31, 2023
8304959: Public API in javafx.css.Match should not return private API class PseudoClassState
Reviewed-by: kcr, angorya
·
25+821-ga
1 parent 2a6e48f commit 3fa02ee

File tree

8 files changed

+161
-130
lines changed

8 files changed

+161
-130
lines changed
 

‎modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java

+54-36
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -24,22 +24,23 @@
2424
*/
2525
package com.sun.javafx.css;
2626

27+
import java.util.AbstractSet;
28+
import java.util.Collection;
29+
import java.util.Iterator;
30+
import java.util.NoSuchElementException;
31+
2732
import com.sun.javafx.collections.SetListenerHelper;
33+
2834
import javafx.beans.InvalidationListener;
2935
import javafx.collections.FXCollections;
3036
import javafx.collections.ObservableSet;
3137
import javafx.collections.SetChangeListener;
3238

33-
import java.util.Collection;
34-
import java.util.Iterator;
35-
import java.util.NoSuchElementException;
36-
37-
3839
/**
3940
* Pseudo-class state and style-classes are represented as bits in a long[]
4041
* which makes matching faster.
4142
*/
42-
abstract class BitSet<T> implements ObservableSet<T> {
43+
abstract class BitSet<T> extends AbstractSet<T> implements ObservableSet<T> {
4344

4445
/** Create an empty set of T */
4546
protected BitSet () {
@@ -229,10 +230,14 @@ public boolean contains(Object o) {
229230
/** {@inheritDoc} */
230231
@Override
231232
public boolean containsAll(Collection<?> c) {
233+
if (this.getClass() != c.getClass()) {
234+
for (Object obj : c) {
235+
if (!contains(obj)) {
236+
return false;
237+
}
238+
}
232239

233-
if (c == null || this.getClass() != c.getClass()) {
234-
// this not modified!
235-
return false;
240+
return true;
236241
}
237242

238243
BitSet other = (BitSet)c;
@@ -258,10 +263,14 @@ public boolean containsAll(Collection<?> c) {
258263
/** {@inheritDoc} */
259264
@Override
260265
public boolean addAll(Collection<? extends T> c) {
266+
if (this.getClass() != c.getClass()) {
267+
boolean modified = false;
261268

262-
if (c == null || this.getClass() != c.getClass()) {
263-
// this not modified!
264-
return false;
269+
for (T obj : c) {
270+
modified |= add(obj);
271+
}
272+
273+
return modified;
265274
}
266275

267276
boolean modified = false;
@@ -330,10 +339,19 @@ public boolean addAll(Collection<? extends T> c) {
330339
/** {@inheritDoc} */
331340
@Override
332341
public boolean retainAll(Collection<?> c) {
342+
if (this.getClass() != c.getClass()) {
343+
boolean modified = false;
333344

334-
if (c == null || this.getClass() != c.getClass()) {
335-
clear();
336-
return true;
345+
for (Iterator<T> iterator = this.iterator(); iterator.hasNext();) {
346+
T obj = iterator.next();
347+
348+
if (!c.contains(obj)) {
349+
iterator.remove();
350+
modified = true;
351+
}
352+
}
353+
354+
return modified;
337355
}
338356

339357
boolean modified = false;
@@ -412,10 +430,14 @@ public boolean retainAll(Collection<?> c) {
412430
/** {@inheritDoc} */
413431
@Override
414432
public boolean removeAll(Collection<?> c) {
433+
if (this.getClass() != c.getClass()) {
434+
boolean modified = false;
415435

416-
if (c == null || this.getClass() != c.getClass()) {
417-
// this not modified!
418-
return false;
436+
for (Object obj : c) {
437+
modified |= remove(obj);
438+
}
439+
440+
return modified;
419441
}
420442

421443
boolean modified = false;
@@ -495,31 +517,28 @@ public void clear() {
495517

496518
@Override
497519
public int hashCode() {
498-
int hash = 7;
499-
if (bits.length > 0) {
500-
for (int n = 0; n < bits.length; n++) {
501-
final long mask = bits[n];
502-
hash = 71 * hash + (int)(mask ^ (mask >>> 32));
503-
}
504-
}
505-
return hash;
520+
// Note: overridden because equals is overridden; both equals and hashCode MUST
521+
// respect the Set contract to interact correctly with sets of other types!
522+
return super.hashCode();
506523
}
507524

508525
@Override
509526
public boolean equals(Object obj) {
510-
511-
if (this == obj) {
527+
// Note: overridden to provide a fast path; both equals and hashCode MUST respect
528+
// the Set contract to interact correctly with sets of other types!
529+
if (obj == this) {
512530
return true;
513531
}
514-
515-
if (obj == null || this.getClass() != obj.getClass()) {
516-
return false;
532+
if (getClass() == obj.getClass()) { // fast path if other is exact same type of BitSet
533+
return equalsBitSet((BitSet<?>) obj);
517534
}
518535

519-
final BitSet other = (BitSet) obj;
536+
return super.equals(obj);
537+
}
520538

521-
final int a = this.bits != null ? this.bits.length : 0;
522-
final int b = other.bits != null ? other.bits.length : 0;
539+
private boolean equalsBitSet(BitSet<?> other) {
540+
int a = this.bits != null ? this.bits.length : 0;
541+
int b = other.bits != null ? other.bits.length : 0;
523542

524543
if (a != b) return false;
525544

@@ -626,4 +645,3 @@ private void notifyObservers(T element, boolean removed) {
626645
}
627646
}
628647
}
629-

‎modules/javafx.graphics/src/main/java/javafx/css/CompoundSelector.java

+4-7
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,6 @@ public List<SimpleSelector> getSelectors() {
9797
: Collections.EMPTY_LIST;
9898
}
9999

100-
private CompoundSelector() {
101-
this(null, null);
102-
}
103-
104-
105100
@Override public Match createMatch() {
106101
final PseudoClassState allPseudoClasses = new PseudoClassState();
107102
int idCount = 0;
@@ -110,7 +105,7 @@ private CompoundSelector() {
110105
for(int n=0, nMax=selectors.size(); n<nMax; n++) {
111106
Selector sel = selectors.get(n);
112107
Match match = sel.createMatch();
113-
allPseudoClasses.addAll(match.pseudoClasses);
108+
allPseudoClasses.addAll(match.getPseudoClasses());
114109
idCount += match.idCount;
115110
styleClassCount += match.styleClassCount;
116111
}
@@ -151,7 +146,9 @@ private CompoundSelector() {
151146
final Set<PseudoClass> pseudoClassIn = tempStates[n];
152147

153148
if (pseudoClassOut != null) {
154-
pseudoClassOut.addAll(pseudoClassIn);
149+
if (pseudoClassIn != null) {
150+
pseudoClassOut.addAll(pseudoClassIn);
151+
}
155152
} else {
156153
triggerStates[n] = pseudoClassIn;
157154
}

‎modules/javafx.graphics/src/main/java/javafx/css/Match.java

+21-16
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -25,10 +25,11 @@
2525

2626
package javafx.css;
2727

28-
import com.sun.javafx.css.PseudoClassState;
29-
3028
import static javafx.geometry.NodeOrientation.INHERIT;
3129

30+
import java.util.Collections;
31+
import java.util.Objects;
32+
import java.util.Set;
3233

3334
/**
3435
* Used by {@link Rule} to determine whether or not the selector applies to a
@@ -40,24 +41,26 @@
4041
*/
4142
public final class Match implements Comparable<Match> {
4243

43-
final Selector selector;
44-
final PseudoClassState pseudoClasses;
45-
final int idCount;
44+
private final Selector selector;
45+
private final Set<PseudoClass> pseudoClasses;
46+
4647
final int styleClassCount;
48+
final int idCount;
4749

4850
// CSS3 spec gives weight to id count, then style class count,
4951
// then pseudoclass count, and finally matching types (i.e., java name count)
5052
final int specificity;
5153

52-
Match(final Selector selector, PseudoClassState pseudoClasses, int idCount, int styleClassCount) {
53-
assert selector != null;
54+
Match(final Selector selector, Set<PseudoClass> pseudoClasses, int idCount, int styleClassCount) {
55+
Objects.requireNonNull(selector);
56+
Objects.requireNonNull(pseudoClasses);
57+
5458
this.selector = selector;
5559
this.idCount = idCount;
5660
this.styleClassCount = styleClassCount;
57-
this.pseudoClasses = pseudoClasses;
58-
int nPseudoClasses = pseudoClasses != null ? pseudoClasses.size() : 0;
59-
if (selector instanceof SimpleSelector) {
60-
final SimpleSelector simple = (SimpleSelector)selector;
61+
this.pseudoClasses = Collections.unmodifiableSet(pseudoClasses);
62+
int nPseudoClasses = pseudoClasses.size();
63+
if (selector instanceof SimpleSelector simple) {
6164
if (simple.getNodeOrientation() != INHERIT) {
6265
nPseudoClasses += 1;
6366
}
@@ -67,17 +70,19 @@ public final class Match implements Comparable<Match> {
6770

6871
/**
6972
* Gets the {@code Selector}.
70-
* @return the {@code Selector}
73+
*
74+
* @return the {@code Selector}, never {@code null}
7175
*/
7276
public Selector getSelector() {
7377
return selector;
7478
}
7579

7680
/**
77-
* Gets the pseudo class state.
78-
* @return the pseudo class state
81+
* Gets the pseudo class states as an immutable set.
82+
*
83+
* @return the pseudo class state, never {@code null}
7984
*/
80-
public PseudoClassState getPseudoClasses() {
85+
public Set<PseudoClass> getPseudoClasses() {
8186
return pseudoClasses;
8287
}
8388

‎modules/javafx.graphics/src/main/java/javafx/css/Selector.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ public int getOrdinal() {
9393

9494
/**
9595
* Creates a {@code Match}.
96-
* @return match
96+
*
97+
* @return a match, never {@code null}
9798
*/
9899
public abstract Match createMatch();
99100

‎modules/javafx.graphics/src/main/java/javafx/css/SimpleSelector.java

+27-48
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ public List<String> getStyleClasses() {
8686
}
8787

8888
/**
89-
* Gets the {@code Set} of {@code StyleClass}es of the {@code Selector}.
89+
* Gets the immutable {@code Set} of {@code StyleClass}es of the {@code Selector}.
9090
* @return the {@code Set} of {@code StyleClass}es
9191
*/
9292
public Set<StyleClass> getStyleClassSet() {
@@ -96,9 +96,9 @@ public Set<StyleClass> getStyleClassSet() {
9696
/**
9797
* styleClasses converted to a set of bit masks
9898
*/
99-
final private StyleClassSet styleClassSet;
99+
private final Set<StyleClass> styleClassSet;
100100

101-
final private String id;
101+
private final String id;
102102

103103
/**
104104
* Gets the value of the selector id.
@@ -108,35 +108,13 @@ public String getId() {
108108
return id;
109109
}
110110

111-
// a mask of bits corresponding to the pseudoclasses
112-
final private PseudoClassState pseudoClassState;
111+
// a mask of bits corresponding to the pseudoclasses (immutable)
112+
private final Set<PseudoClass> pseudoClassState;
113113

114114
Set<PseudoClass> getPseudoClassStates() {
115115
return pseudoClassState;
116116
}
117117

118-
/**
119-
* Gets an immutable list of {@code String}s of pseudo classes of the {@code Selector}
120-
* @return an immutable list of {@code String}s
121-
*/
122-
List<String> getPseudoclasses() {
123-
124-
final List<String> names = new ArrayList<>();
125-
126-
Iterator<PseudoClass> iter = pseudoClassState.iterator();
127-
while (iter.hasNext()) {
128-
names.add(iter.next().getPseudoClassName());
129-
}
130-
131-
if (nodeOrientation == RIGHT_TO_LEFT) {
132-
names.add("dir(rtl)");
133-
} else if (nodeOrientation == LEFT_TO_RIGHT) {
134-
names.add("dir(ltr)");
135-
}
136-
137-
return Collections.unmodifiableList(names);
138-
}
139-
140118
// true if name is not a wildcard
141119
final private boolean matchOnName;
142120

@@ -168,41 +146,42 @@ public NodeOrientation getNodeOrientation() {
168146
// then match needs to check name
169147
this.matchOnName = (name != null && !("".equals(name)) && !("*".equals(name)));
170148

171-
this.styleClassSet = new StyleClassSet();
149+
Set<StyleClass> scs = new StyleClassSet();
172150

173-
int nMax = styleClasses != null ? styleClasses.size() : 0;
174-
for(int n=0; n<nMax; n++) {
151+
if (styleClasses != null) {
152+
for (int n = 0; n < styleClasses.size(); n++) {
175153

176-
final String styleClassName = styleClasses.get(n);
177-
if (styleClassName == null || styleClassName.isEmpty()) continue;
154+
final String styleClassName = styleClasses.get(n);
155+
if (styleClassName == null || styleClassName.isEmpty()) continue;
178156

179-
final StyleClass styleClass = StyleClassSet.getStyleClass(styleClassName);
180-
this.styleClassSet.add(styleClass);
157+
scs.add(StyleClassSet.getStyleClass(styleClassName));
158+
}
181159
}
182160

161+
this.styleClassSet = Collections.unmodifiableSet(scs);
183162
this.matchOnStyleClass = (this.styleClassSet.size() > 0);
184163

185-
this.pseudoClassState = new PseudoClassState();
164+
PseudoClassState pcs = new PseudoClassState();
165+
NodeOrientation dir = NodeOrientation.INHERIT;
186166

187-
nMax = pseudoClasses != null ? pseudoClasses.size() : 0;
167+
if (pseudoClasses != null) {
168+
for (int n = 0; n < pseudoClasses.size(); n++) {
188169

189-
NodeOrientation dir = NodeOrientation.INHERIT;
190-
for(int n=0; n<nMax; n++) {
170+
final String pclass = pseudoClasses.get(n);
171+
if (pclass == null || pclass.isEmpty()) continue;
191172

192-
final String pclass = pseudoClasses.get(n);
193-
if (pclass == null || pclass.isEmpty()) continue;
173+
// TODO: This is not how we should handle functional pseudo-classes in the long-run!
174+
if ("dir(".regionMatches(true, 0, pclass, 0, 4)) {
175+
final boolean rtl = "dir(rtl)".equalsIgnoreCase(pclass);
176+
dir = rtl ? RIGHT_TO_LEFT : LEFT_TO_RIGHT;
177+
continue;
178+
}
194179

195-
// TODO: This is not how we should handle functional pseudo-classes in the long-run!
196-
if ("dir(".regionMatches(true, 0, pclass, 0, 4)) {
197-
final boolean rtl = "dir(rtl)".equalsIgnoreCase(pclass);
198-
dir = rtl ? RIGHT_TO_LEFT : LEFT_TO_RIGHT;
199-
continue;
180+
pcs.add(PseudoClassState.getPseudoClass(pclass));
200181
}
201-
202-
final PseudoClass pseudoClass = PseudoClassState.getPseudoClass(pclass);
203-
this.pseudoClassState.add(pseudoClass);
204182
}
205183

184+
this.pseudoClassState = Collections.unmodifiableSet(pcs);
206185
this.nodeOrientation = dir;
207186
this.id = id == null ? "" : id;
208187
// if id is not null and not empty, then match needs to check id

‎modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java

+8-2
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,10 @@ static CssStyleHelper createStyleHelper(final Node node) {
131131
node.styleHelper.cacheContainer.fontSizeCache.clear();
132132
}
133133
node.styleHelper.cacheContainer.forceSlowpath = true;
134-
node.styleHelper.triggerStates.addAll(triggerStates[0]);
134+
135+
if (triggerStates[0] != null) {
136+
node.styleHelper.triggerStates.addAll(triggerStates[0]);
137+
}
135138

136139
updateParentTriggerStates(node, depth, triggerStates);
137140
return node.styleHelper;
@@ -171,7 +174,10 @@ static CssStyleHelper createStyleHelper(final Node node) {
171174
}
172175

173176
final CssStyleHelper helper = new CssStyleHelper();
174-
helper.triggerStates.addAll(triggerStates[0]);
177+
178+
if (triggerStates[0] != null) {
179+
helper.triggerStates.addAll(triggerStates[0]);
180+
}
175181

176182
updateParentTriggerStates(node, depth, triggerStates);
177183

‎modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/BitSetTest.java

+41
Original file line numberDiff line numberDiff line change
@@ -26,17 +26,23 @@
2626
package test.com.sun.javafx.css;
2727

2828
import static org.junit.jupiter.api.Assertions.assertEquals;
29+
import static org.junit.jupiter.api.Assertions.assertNotEquals;
2930
import static org.junit.jupiter.api.Assertions.assertTrue;
3031

32+
import java.util.Set;
3133
import java.util.concurrent.atomic.AtomicInteger;
3234

3335
import org.junit.jupiter.api.Test;
3436

3537
import com.sun.javafx.css.BitSetShim;
38+
import com.sun.javafx.css.PseudoClassState;
39+
import com.sun.javafx.css.PseudoClassStateShim;
40+
import com.sun.javafx.css.StyleClassSet;
3641

3742
import javafx.beans.InvalidationListener;
3843
import javafx.collections.SetChangeListener;
3944
import javafx.css.PseudoClass;
45+
import javafx.css.StyleClass;
4046

4147
public class BitSetTest {
4248
private final BitSetShim<PseudoClass> set = BitSetShim.getPseudoClassInstance();
@@ -134,4 +140,39 @@ void listenerManagementForSetChangeListenerShouldWorkCorrectly() {
134140

135141
assertEquals(0, changed.getAndSet(0)); // not called
136142
}
143+
144+
@Test
145+
void twoNonEmptyBitSetsWithSamePatternAndSizeShouldNotBeConsideredEqualsWhenElementTypesAreDifferent() {
146+
StyleClassSet set1 = new StyleClassSet();
147+
PseudoClassState set2 = new PseudoClassState();
148+
149+
PseudoClass pseudoClass = PseudoClass.getPseudoClass("abc");
150+
151+
int index = PseudoClassStateShim.pseudoClassMap.get(pseudoClass.getPseudoClassName());
152+
153+
set1.add(new StyleClass("xyz", index)); // no idea why this is public API, but I'll take it
154+
set2.add(pseudoClass);
155+
156+
/*
157+
* The two sets above contain elements of different types (PseudoClass and StyleClass)
158+
* and therefore should never be equal, despite their bit pattern being the same:
159+
*/
160+
161+
assertNotEquals(set1, set2);
162+
}
163+
164+
@Test
165+
void twoEmptyBitSetsShouldBeEqual() {
166+
167+
/*
168+
* Per Set contract, the empty set is equal to any other empty set.
169+
*/
170+
171+
assertEquals(new StyleClassSet(), new PseudoClassState());
172+
assertEquals(new PseudoClassState(), new StyleClassSet());
173+
assertEquals(Set.of(), new PseudoClassState());
174+
assertEquals(new PseudoClassState(), Set.of());
175+
assertEquals(Set.of(), new StyleClassSet());
176+
assertEquals(new StyleClassSet(), Set.of());
177+
}
137178
}

‎modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/PseudoClassTest.java

+4-20
Original file line numberDiff line numberDiff line change
@@ -605,18 +605,10 @@ public void testPseudoClassState_retainAll_withEmptyStates() {
605605
}
606606

607607
@Test
608-
public void testPseudoClassState_retainAll_withNullArg() {
609-
608+
public void testPseudoClassState_retainAll_throwsWithNullArg() {
610609
PseudoClassState aStates = new PseudoClassState();
611-
PseudoClassState bStates = null;
612-
BitSetShim.retainAll(aStates, bStates);
613-
List<PseudoClass> states = new ArrayList<>();
614-
Iterator<PseudoClass> iter = BitSetShim.iterator(aStates);
615-
while (iter.hasNext()) {
616-
states.add(iter.next());
617-
}
618-
assertEquals(0, states.size(), 0.000001);
619610

611+
assertThrows(NullPointerException.class, () -> BitSetShim.retainAll(aStates, null));
620612
}
621613

622614
@Test
@@ -711,18 +703,10 @@ public void testPseudoClassState_addAll_withEmptyStates() {
711703
}
712704

713705
@Test
714-
public void testPseudoClassState_addAll_withNullArgs() {
715-
706+
public void testPseudoClassState_addAll_throwsWithNullArgs() {
716707
PseudoClassState aStates = new PseudoClassState();
717-
PseudoClassState bStates = null;
718-
BitSetShim.addAll(aStates, bStates);
719-
List<PseudoClass> states = new ArrayList<>();
720-
Iterator<PseudoClass> iter = BitSetShim.iterator(aStates);
721-
while (iter.hasNext()) {
722-
states.add(iter.next());
723-
}
724-
assertEquals(0, states.size(), 0.000001);
725708

709+
assertThrows(NullPointerException.class, () -> BitSetShim.addAll(aStates, null));
726710
}
727711

728712
@Test

0 commit comments

Comments
 (0)
Please sign in to comment.