On Wed, 22 Apr 2020 12:03:34 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
>> Closed focused Stages are not collected with Monocle >> >> This commit adds a unit-test and a fix for collecting focused closed stages. >> >> ticket: https://bugs.openjdk.java.net/browse/JDK-8241840 > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8241840 > The tests are now reused for native and monocle tests. The fix to Monocle looks good. So does the test (I left some minor comments). I can confirm that the test catches the leak (it passes with your fix and fails without it). My only question is regarding the change to `Window.java`. tests/system/src/test/java/test/javafx/stage/FocusedWindowMonocleTest.java line 56: > 55: } > 56: } Minor: Add missing newline tests/system/src/test/java/test/javafx/stage/FocusedWindowNativeTest.java line 52: > 51: } > 52: } Minor: Add missing newline tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java line 45: > 44: @Ignore > 45: abstract public class FocusedWindowTestBase { > 46: The preferred order is `public abstract`. Also, there is no need for an `@Ignore` here. tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java line 50: > 49: > 50: > 51: public static void initFXBase() throws Exception { Minor: remove extra blank line. tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java line 60: > 59: static WeakReference<Stage> closedFocusedStageWeak = null; > 60: static Stage closedFocusedStage = null; > 61: These two can be instance fields (which is usually preferred for test variables). tests/system/src/test/java/test/javafx/stage/FocusedWindowTestBase.java line 106: > 105: } > 106: } Minor: Add missing newline ------------- PR: https://git.openjdk.java.net/jfx/pull/153
