RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]

[John Hendrikx]

On Wed, 18 Oct 2023 18:07:53 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Sai Pradeep Dandem has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8185831: Fixed test failing issues and code review comments
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1993:
> 
>> 1991:      */
>> 1992:     List<Node> lookupAll(Selector selector, List<Node> results) {
>> 1993:         if (selectorMatches(selector)) {
> 
> line 1990 might be incorrect: the return value **can** be null.
> We probably should just correct the javadoc, and since it's a private API no CSR is needed.  All callers of this method do check for null.

I must say I'm again baffled at how this is implemented.  

Red flag one: it uses a `LinkedList` which are known to be rarely the right choice, and I doubt this case is any different.  

Red flag two: a `List` is converted to a `Set`; being backed by a `LinkedList` means set type operations will be terribly slow if that list has any kind of size.  

Red flag three: The choice to return a `Set` in the public API seems to be only motivated to indicate there won't be any duplicates, which is a terrible reason.

Red flag four: `UnmodifiableListSet` talks about insertion speed and hashing overhead, yet uses a `LinkedList` which are slower than `ArrayList` when it comes to insertion speed (not to mention that they require more object allocations and more memory(!)).

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1364940463