RFR: 8252935: Add treeShowing listener only when needed

[John Hendrikx]

On Fri, 17 Apr 2020 17:18:00 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> These listeners exist for a good reason. Removing them will almost certainly cause regressions in behavior. See
>> [JDK-8090322](https://bugs.openjdk.java.net/browse/JDK-8090322) as well as the follow-on fixes (since that was a rather
>> involved fix in the first place).
>
> @kevinrushforth I don't propose to remove them, only to move them to where they are needed.
> 
> Currently they are part of Node, which means all nodes, whether they need to know the state of the Window or not are
> registering a listener.  However, only PopupWindow and the ProgressIndicatorSkin are registering a listener on this
> property (other uses are limited to a simple "isTreeShowing" checks which can be determined directly).    It is very
> wasteful to have all nodes register this listener, as there are potentially tens of thousands of nodes.  All of these
> nodes are listening on the exact same two properties (when there is only one Scene and Window), causing huge listener
> lists there.  The listener is not registered in a lazy fashion  (ie, only registered if something else is listening to
> the property downstream, like reactfx would do).    This also means that when a Scene change or Window showing change
> occurs, thousands of nodes will receive a change event, but only 2 types of nodes are actually interested.  The other
> nodes are just doing a lot of house keeping to keep watching the correct property (as there is an indirection from
> Scene to Window).  Therefore my proposal is to have the two cases involved register their own listener on Scene and
> Window.  There will not be any regressions.  The two tests that currently fail in this PR are expected to fail as I
> commented out the listener code in the classes involved, but that can easily be fixed by adding the correct listeners
> there.  I'll update it so you can see all tests will pass.

@kevinrushforth I've updated this PR now with proper listeners added in again for PopupWindow and
ProgressIndicatorSkin.  In short, the functionality to support the `treeShowingProperty` has been extracted to a
separate class `TreeShowingExpression` which is now used by the two classes mentioned.

All tests pass, including the memory leak tests that failed before.

The issue JDK-8090322 you mentioned actually cautioned for not adding such listeners for all nodes and seemed to
propose the kind of solution I constructed here with a separate class for the `treeShowingProperty`:

> This is too expensive to calculate for all nodes by default. So the simplest way to provide it would be a special
> binding implementation or a util class. Where you create a instance and pass in the node you are interested in. It can
> then register listeners all the way up the tree and listen to what it needs.

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

PR: https://git.openjdk.java.net/jfx/pull/185