RFR: 8242621: TabPane: Memory leak when switching skin [v3]

[Jeanette Winzenburg]

On Wed, 6 Jan 2021 12:22:17 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 289:
>> 
>>> 287:       assertNull(tabPane.getSkin());
>>> 288:       assertEquals(0, tabPane.getChildrenUnmodifiable().size());
>>> 289:     }
>> 
>> Same as above: what's the reason to actually show?
>> 
>> And just curious:  why is this passing already before the fix?
>
> Corrected to use installDefaultSkin().
> It works without this fix is because the children are cleared([here](https://github.com/openjdk/jfx/blob/e74f679fda6f03ee8449836147815fdaafb5d626/modules/javafx.controls/src/main/java/javafx/scene/control/Control.java#L300)) in Control class when the skin is set to null.

thanks for the info :)

>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 1577:
>> 
>>> 1575:             listener.dispose();
>>> 1576:             inner.getChildren().clear();
>>> 1577:             getChildren().clear();
>> 
>> I'm aware this is internal and old api but .. could we consider cleaning it up a bit?
>> 
>> - its task is to cleanup itself, doing whatever it deems is needed (here: removing listeners and children), so removeListeners is a misnomer as it describes only part of it. A (well-established) name for the task would be dispose
>> - passing in the tab is .. strange, as can be seen in its usage in TabHeaderArea.dispose:  `header.removeListeners(header.getTab())` Actually, it would be an error to call the method with a `tab != header.getTab()`
>
> +1, passing `tab` is unnecessary here. I have removed the parameter and the method is now renamed as `dispose()`. 
> Also it is not required to clear the children of `TabHeaderSkin`, so have removed
> `inner.getChildren().clear();` and `getChildren().clear();` calls from this method.

good catch :)

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

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