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

Ambarish Rapte arapte at openjdk.java.net
Wed Jan 6 12:16:01 UTC 2021


On Tue, 5 Jan 2021 16:03:42 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/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 275:
> 
>> 273:       assertEquals(3, tabPane.getChildrenUnmodifiable().size());
>> 274:     }
>> 275: 
> 
> hmm .. looks a bit unusual (could be me, though :)
> 
> - why showControl? just install/replace the skin should be enough?
> - the outcome of testing an illegal state (after instantiating the skin and before really setting it as the control's skin at which time we have a skin : control relation of 2:1) is unspecified. F.i. there are skins that replace all children in the constructor (which may or may not be a good idea) that would fail a test similar to this.
> 
> How about
> 
>     TabPane tabPane ..
>     installDefaultSkin(tabPane);
>     int children = tabPane.getxxChildren().size();
>     replaceSkin(tabPane);
>     assertEquals(children, tabPane.getxxChildren().size());
> 
> If we decide to go without showing, the other tests should be changed as well :)

1. `showControl` is not required. It is now changed to `installDefaultSkin` in all tests.
2. Test did verify an illegal state, Intention was to keep a note of that illegal state. I have changed it as you suggested.

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

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


More information about the openjfx-dev mailing list