RFR: 8252935: Add treeShowing listener only when needed [v4]

[Ambarish Rapte]

On Mon, 1 Feb 2021 04:44:03 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> This is a PoC for performance testing.  
>> 
>> It contains commented out code in PopupWindow and ProgressIndicatorSkin and two tests are failing because of that.
>> 
>> This code avoids registering two listeners (on Scene and on Window) for each and every Node to support the aforementioned classes.  The complete change should update these two classes to add their own listeners instead.
>
> John Hendrikx has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - Add missing TreeShowingExpressionTest which also tests SubScene
>  - Also dispose listeners on Window/Showing in dispose
>  - Fix review comments
>  - Update copyrights

Added couple minor comments and a suggestion.
Rest looks good to me, Observed no failures. It would be a great performance improvement for large scene applications.

modules/javafx.graphics/src/test/java/test/javafx/scene/TreeShowingExpressionTest.java line 318:

> 316:         assertNull(state.getAndSet(null));
> 317:     }
> 318: }

Please add a new line here at end of file.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/TreeShowingExpression.java line 2:

> 1: /*
> 2:  * Copyright (c) 2013, 2021, Oracle and/or its affiliates. All rights reserved.

It should be changed to -> `Copyright (c) 2021, Oracle `

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/TreeShowingExpression.java line 89:

> 87:         NodeHelper.treeVisibleProperty(node).addListener(windowShowingChangedListener);
> 88: 
> 89:         nodeSceneChangedListener.changed(null, null, node.getScene());  // registers listeners on Window/Showing

We do not follow this pattern of calling the changed() or invalidated() methods. Instead we directly add the necessary calls at the time of initialisation and disposal. I think the pattern should be followed here too.
For reference, the pattern is followed in all Skin classes. For example [here](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L148) in ButtonSkin, we do not make a call to `sceneChangeListener.changed()`, instead the necessary calls are directly made on [next lines](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L151) and then [opposite](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L179) calls in dispose().

For this line 89 needs to be replaced by following code, (with additional null checks)
node.getScene().windowProperty().addListener(sceneWindowChangedListener);
node.getScene().getWindow().showingProperty().addListener(windowShowingChangedListener);
updateTreeShowing();

and similarly they should be removed in dispose().

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

Changes requested by arapte (Reviewer).

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