On Tue, 26 Nov 2019 09:23:58 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
> The pull request has been updated with a complete new set of changes > (possibly due to a rebase). > > ---------------- > > Commits: > - 44774dfb: JDK-8200224 > - d1309fb6: JDK-8200224 > - 53a66b8f: JDK-8200224 > - 0be3cb5b: JDK-8200224 > - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into > JDK-8200224 > - 5cd96a56: JDK-8200224 > - 85c87628: JDK-8200224 > - 314e423c: JDK-8200224 > - 725e5fef: JDK-8200224 > > Changes: https://git.openjdk.java.net/jfx/pull/25/files > Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.03 > Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 > Stats: 272 lines in 3 files changed: 147 ins; 120 del; 5 mod > Patch: https://git.openjdk.java.net/jfx/pull/25.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 To follow-up on my previous comment here are the requested changes: 1. Restore ` tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java`, which was removed by your last commit, so that it doesn't show up in the PR. 2. In the new test, add a dummy `JFXPanel` to the `JPanel` so that the test `JFXPanel` is the second one added (see my inline comments). This will allow the test to catch the error on Windows by failing without your fix. 3. A few other inline comments on the new test. tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 2: > 1: /* > 2: * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved. > 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. That should be `2019` tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 35: > 34: > 35: > 36: import javax.swing.JFrame; You only need one blank line here. tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 94: > 93: > 94: class TestFXPanel extends JFXPanel { > 95: protected void processMouseEventPublic(MouseEvent e) { This class can be `static` tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 108: > 107: > 108: SwingUtilities.invokeLater(() -> { > 109: TestFXPanel fxPnl = new TestFXPanel(); Add the following here: JFXPanel dummyFXPanel = new JFXPanel(); dummyFXPanel.setPreferredSize(new Dimension(100, 100)); tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 112: > 111: JFrame jframe = new JFrame(); > 112: JPanel jpanel = new JPanel(); > 113: jpanel.add(fxPnl); Add the following here: jpanel.add(dummyFXPanel); tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 118: > 117: > 118: Platform.runLater(() -> { > 119: Group grp = new Group(); Add the following here: Scene dummyScene = new Scene(new Group()); dummyFXPanel.setScene(dummyScene); tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 138: > 137: > 138: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) { > 139: throw new Exception(); This `if` test can be written more succinctly as: assertTrue(firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)); tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 72: > 71: @BeforeClass > 72: public static void doSetupOnce() { > 73: // Start the Application If you add `throws Exception` then you don't need the try/catch around the `launchLatch.await`. tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 119: > 118: Platform.runLater(() -> { > 119: Group grp = new Group(); > 120: Scene scene = new Scene(new Group()); This variable is unused. ---------------- Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/25
