RFR: 8242621: TabPane: Memory leak when switching skin [v3]
Ambarish Rapte
arapte at openjdk.java.net
Wed Jan 6 11:29:00 UTC 2021
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
More information about the openjfx-dev
mailing list