RFR: JDK-8323706 Move SimpleSelector and CompoundSelector to internal packages [v2]

[John Hendrikx]

On Fri, 19 Jan 2024 09:52:23 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 179:
>> 
>>> 177:      * @throws IOException if reading from {@code DataInputStream} fails
>>> 178:      */
>>> 179:     protected static Selector readBinary(int bssVersion, DataInputStream is, String[] strings)
>> 
>> This is an API addition, so will need an `@since 23`. Since you are adding new API here, should it be an instance method rather than a static method? Or is is called without having an instance of Selector in some cases?
>
> I'll add the `@since 23`, however it can't be called by anyone except FX itself.
> 
> Some background: the readBinary/writeBinary methods are ultimately called via the publicly accessible methods `loadBinary` and `saveBinary` in `javafx.css.Stylesheet`.  
> 
> The reason it needs to be `protected` now is because `CompoundSelector` is using this (but IMHO, it shouldn't have).  Why I say it shouldn't?  That's because it already knows what it will be reading will all be `SimpleSelector`s, as it stores a counter (a `short`) and then loads that many `SimpleSelector`s.  However, by not taking the direct route of using `SimpleSelector#readBinary` (and the corresponding `SimpleSelector#writeBinary`) there is an additional `byte` prefix indicating the type of selector -- this isn't needed and wastes some space in the binary format.
> 
> Changing that now however would make the binary format incompatible with earlier versions, so I think making this `protected` is a reasonable solution.  It also mirrors the `writeBinary` method then, which was already `protected`.

I missed something in my above evaluation.  The counterpart method `writeBinary` is not `static`.  Although that makes sense as in that case you do have an instance you wish to write, it does make the read/write API asymmetric.

It's not possible to make `readBinary` an instance method though as it is the method that is creating those instances.

The other way around is possible (make `writeBinary` static), but it would then again need a pattern match to determine the correct static `writeBinary` to call when writing an arbitrary `Selector`.  However, this would have allowed `CompoundSelector` to call a static version of `SimpleSelector#writeBinary` and `readBinary`, avoiding the extra identifying byte in the binary format, and avoiding the cast when reading it back.

The read/write loops below:

+
        final int nSelectors = is.readShort();
        final List<SimpleSelector> selectors = new ArrayList<>();
        for (int n=0; n<nSelectors; n++) {
            selectors.add((SimpleSelector)Selector.readBinary(bssVersion, is,strings));
        }
+
        os.writeShort(selectors.size());   // writes the number of SimpleSelectors to the binary stream
        for (int n=0; n< selectors.size(); n++) selectors.get(n).writeBinary(os,stringStore);  // writes each individually

Would then have become:

+
        final int nSelectors = is.readShort();
        final List<SimpleSelector> selectors = new ArrayList<>();
        for (int n=0; n<nSelectors; n++) {
            selectors.add(SimpleSelector.readBinary(bssVersion, is,strings));  // cast is gone
        }
+
        os.writeShort(selectors.size());   // writes the number of SimpleSelectors to the binary stream
        for (int n=0; n< selectors.size(); n++) SimpleSelector.writeBinaryStatic(selectors.get(n), os, stringStore);  // writes each individually

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1458762627