On Thu, 11 Mar 2021 21:58:40 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Since this touches CSS, it needs a second reviewer. > > I think others can review this. I do have a couple questions: > 1. In general, I don't like the idea of just making everything a weak > reference, since it often masks a design issue. Two exceptions are for caches > and for back references to a "parent" or controlling object that has a strong > reference to "this" object (most of our weak listeners fall into this latter > pattern). It sounds like latter case also applies here. Can you confirm that? > 2. Have you verified that all the places that use the fields that are now > WeakReferences are prepared to deal with `get()` returning a null object? About whether we should use WeakReference here or not. It definitely falls into the exception for referring to the Parrent of a Node. (Or to the Parent in a more abstract sense, I think it can also be the parent of the parent, or even from another SceneGraph.) I don't know the CSS code very well, so I currently don't have the knowledge how to change it. But if we would change this variable, whenever the node is removed from the SceneGraph, my concern would be that it would have an unfavorable complexity. Currently (I hope) the complexity of removing a Node from the SceneGraph is O(1). If we would remove the stylehelper from the node, then the complexity would be O(<nodes-in-the-tree>). The current change assumes that we don't process the CSS, when a node is removed from the SG. This is why it isn't checked for null. I would argue, if this causes an Error, then it just reveals another issue, which would be preferable over a more complicated fix, and also changing the complexity of removing nodes from the SG. ------------- PR: https://git.openjdk.java.net/jfx/pull/424
