[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: JDK-8304959: Public API in javafx.css.Match should not return private API class PseudoClass
From:       Kevin Rushforth <kcr () openjdk ! org>
Date:       2023-03-31 13:27:30
Message-ID: lkV_1raENFNDYZf2yiGjXp-AxliUrhjuXksC1FjkSBs=.ee809126-6936-45fc-8ba5-81a1bb4c32db () github ! com
[Download RAW message or body]

On Thu, 30 Mar 2023 23:59:50 GMT, John Hendrikx <jhendrikx@openjdk.org> wrote:

> > I think that `Match` is supposed to be immutable, given the non-public \
> > constructor.  Match itself will never change the set (and nothing else will) so \
> > making it observable seems unnecessary. 
> > However, this was not correctly spec'd, and you can change a `Match` now, and \
> > since it doesn't make a copy, you'd be changing the pseudo classes of whatever \
> > selector created it.  Note that `SimpleSelector` does make a copy, and goes to \
> > great pains to not expose anything mutable, but exposes it accidentally via \
> > `Match`. 
> > In other words, `simpleSelector.createMatch().getPseudoClasses().clear()` would \
> > break the Selectors encapsulation. 
> > I think it's best to close that loophole.  If you agree, I can document this \
> > method that it returns an immutable set, which is also what I assumed would be \
> > the case in my other PR where I made many of these immutable.
> 
> The CSS API baffles me a bit.  It doesn't seem consistent.
> 
> Just now I took a look at the class `SimpleSelector` and `CompoundSelector`. These \
> are public, yet cannot be constructed by users.  They're also not returned anywhere \
> (the closest is `Selector#createSelector` which returns the interface). 
> Essentially this means that `SimpleSelector` and `CompoundSelector` should probably \
> be package private.  Yet, I guess they were made public because \
> `SelectorPartitioning` is doing instanceof checks and is casting to these classes.  \
> But anybody can do that now, and that means that for example \
> `SimpleSelector#getStyleClassSet` is exposed, which returns a mutable set... 
> Reading between the lines though it seems that `SimpleSelector` and \
> `CompoundSelector` were intended to be fully immutable (which makes sense as they \
> represent a style sheet).  Any changes would not be picked up as nothing is \
> observing these properties. 
> I think these loopholes should be closed.
> 
> There are two options IMHO:
> 
> 1. Move `SimpleSelector` and `CompoundSelector` to the com hierarchy.  They can't \
> be publically constructed, and are never returned.  The only way to reach them is \
> by casting. 2. If it's too late for that, then close all loopholes and ensure that \
> these two classes are fully immutable. From what I can see now, only \
> `getStyleClassSet` and the mentioned method in `Match` need closing.  \
> `CompoundSelector` is already immutable.

I'll take a closer look early next week.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1070#discussion_r1154473373


[prev in list] [next in list] [prev in thread] [next in thread]