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

[Ambarish Rapte]

On Tue, 5 Jan 2021 15:42:40 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review update
>
> 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.

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 1735:
> 
>> 1733:             removeListeners();
>> 1734:         }
>> 1735: 
> 
> Wondering why the removal of children down the hierarchy? Shouldn't that be unnecessary as long as the direct children of the skin/control are and all listeners of the grandChildren to the skin are removed?

Corrected, removal of children of `TabContentRegion` is not needed. (same change is also done for `TabHeaderSkin` as mentioned in previous comment)

> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 1846:
> 
>> 1844:             getSkinnable().sideProperty().removeListener(weakSidePropListener);
>> 1845:             getSkinnable().getTabs().removeListener(weakTabsListenerForPopup);
>> 1846:         }
> 
> yet another candidate for rename to dispose (see above)

Corrected.

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

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