RFR: 8345348: CSS media feature queries [v13]

[Michael Strauß]

On Mon, 14 Apr 2025 09:56:43 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   improved implementation of NullCoalescingPropertyBase
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/application/preferences/PreferenceProperties.java line 246:
> 
>> 244:         }
>> 245: 
>> 246:         public synchronized void fireValueChangedIfNecessary() {
> 
> I noticed that you made many methods `synchronized` in this class, but I have trouble seeing why this is.  A short explanation may be in order.
> 
> So, if I had to guess: it's possible to change the value override and/or the updating of all properties from a different thread, which is not the FX thread.  However, what guarantees are now given to listeners (if any)?  The value changed events can now also be triggered from any thread, unless wrapped in a `Platform.runLater`.
> 
> What I mean here is, let's say I listen on `accentColorProperty` of `Platform.Preferences`, on what thread can the change event happen?  The `Platform.Preferences` class does not mention anything special, so I think it is fair to assume it should be on the FX thread, allowing me to make modifications to an active scene graph in my change handler; however that will fail if the change notification was not triggered from the FX thread.

Platform preference change events always happen on the FX thread. The reason for `synchronized` is as follows:

A `Scene` can be created on any thread as per spec. When the scene is created, the `ScenePreferences` class and its associated properties are instantiated, which in turn subscribe internally to the `Platform.Preferences` properties. The synchronization is to protect the addition and removal of listeners.

This is not ideal, though. As soon as we're subscribed to `Platform.Preferences`, the `Scene.Preferences` can receive change notifications on the FX thread, which interferes with the mandated capability to be created on any thread.

I think we have a few options here:
1. Don't subscribe scene preferences to platform preferences until the scene is shown. The downside of this is that we don't know the actual values of the preferences up until that point.
2. With limited synchronization, fetch the current preference values from the platform when the scene is created, but don't subscribe to change notifications until the scene is shown.
3. Specify that scene preference changes can happen on the FX thread, even when the scene is created in a background thread.

> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/MediaRule.java line 129:
> 
>> 127:     public int hashCode() {
>> 128:         return hash;
>> 129:     }
> 
> You're basically saying here that a media rule with or without parent but the same queries is the same.  However, parent is taken into account while writing/reading and evaluating.  I think this deserves a justification.

I might just remove `equals()`/`hashCode()`, because the class is not used in a way where equality would be relevant.

> modules/javafx.graphics/src/main/java/com/sun/javafx/css/media/expression/FunctionExpression.java line 72:
> 
>> 70:     public String toString() {
>> 71:         return "(" + (featureValue != null ? featureName + ": " + featureValue : featureName) + ")";
>> 72:     }
> 
> It's really unusual to override these methods in a record.  What reason do you have for excluding the function from equals/hashCode ?  What does it mean when all fields match but a different function is used?  Where is equality used?

The expression is entirely defined by `featureName`, `featureValue`, and `value`. Since the associated function is usually a method reference, it won't equal other method references (even though the referenced method is the same). There's no point in equating different method references of the same method because this just means that no two expressions are ever equal.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042064946
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042084919
PR Review Comment: https://git.openjdk.org/jfx/pull/1655#discussion_r2042073727