RFR: 8242621: TabPane: Memory leak when switching skin [v3]
Ambarish Rapte
arapte at openjdk.java.net
Wed Jan 6 12:26:59 UTC 2021
On Tue, 5 Jan 2021 16:19:47 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:
> Fix looks good, verified tests failing (except one, commented) before and passing after the fix.
Thanks for the review. I have updated the PR according to all comments. Please take a look. :)
> 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.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 302:
>
>> 300: @Test
>> 301: public void testNPEWhenRemTabAfterSkinIsReplaced() {
>> 302: TabPane tabPane = new TabPane();
>
> Personally I don't like abbreviation in method names, not even test methods. `testNPEWhenRemoveTabAfterSkinIsReplaced` isn't that much longer :)
Corrected.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 311:
>
>> 309: @Test
>> 310: public void testAddRemTabsAfterSkinIsReplaced() {
>> 311: TabPane tabPane = new TabPane();
>
> see above
Corrected
-------------
PR: https://git.openjdk.java.net/jfx/pull/318
More information about the openjfx-dev
mailing list