On Wed, 3 Sep 2025 09:22:17 GMT, Khalid Boulanouare <d...@openjdk.org> wrote:
>> Many Mixing tests failed because the work around click lands on the >> minimizing area in the window control and causes the tests to fail. >> >> This fix changes the width of base frames which allows most of tests to pass. > > Khalid Boulanouare has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 46 additional > commits since the last revision: > > - Merge branch 'openjdk:master' into jdk-8158801 > - Disposes frame at the end of testing. > - Moves latch logic inside ancestor frame block > - Merge branch 'openjdk:master' into jdk-8158801 > - Disposes frames after each AWT component test > - Removes redundant code for centring frames > - Merge branch 'openjdk:master' into jdk-8158801 > - Merge branch 'openjdk:master' into jdk-8158801 > - Centers missed frames in the middle of screen > - Uses KeyboardFocusManager instead of FocusManager > - ... and 36 more: https://git.openjdk.org/jdk/compare/e79df9f1...a62416a9 Changes requested by aivanov (Reviewer). test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 63: > 61: { > 62: multiFramesTest = false; > 63: } I prefer this is done in a constructor rather than a simple initialiser, it just makes the intent clearer. The same applies to setting the initial value of `multiFramesTest` in `SimpleOverlappingTestBase`. `SimpleOverlappingTestBase` has constructors, `GlassPaneOverlappingTestBase` has constructors too. I believe `multiFramesTest` doesn't change, and if you initialise it in a constructor, you can make it `final`, and I'm always for using immutable fields. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 125: > 123: final CountDownLatch latch = new CountDownLatch(1); > 124: f.addFocusListener(new FocusAdapter() { > 125: @Override public void focusGained(FocusEvent e) { Suggestion: f.addFocusListener(new FocusAdapter() { @Override public void focusGained(FocusEvent e) { I prefer a multi-line version of this. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 129: > 127: } > 128: }); > 129: if (testResize) { Suggestion: }); if (testResize) { Let's add a blank line to introduce logical blocks in this long piece of code. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 145: > 143: } else { > 144: f.requestFocusInWindow(); > 145: } Is it possible to add the listener earlier to ensure the latch is released in the focus listener? test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 148: > 146: } > 147: }); > 148: } catch (InterruptedException ex) { Although this is existing code, I propose merging the two catch block into one by using `InterruptedException | InvocationTargetException` as the type. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 165: > 163: return wasLWClicked; > 164: } else { > 165: latch.countDown(); I believe this is not required, the test will not block even if the latch isn't released. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 169: > 167: } > 168: } > 169: protected void cleanup(){ Suggestion: } @Override protected void cleanup(){ Add a blank line between methods, and add the `@Override` annotation. test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 1: > 1: /* Remove `import java.awt.Color;` test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 1: > 1: /* Remove `import java.awt.Color;` test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 1: > 1: /* Could you expand the wildcard imports, please? test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 1: > 1: /* Could you also move `TestPassedException` inside `MixingPanelsResizing` and make it `static`? This would resolve multiple definitions of `TestPassedException` when viewing source for AWT_Mixing in an IDE. test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 1: > 1: /* Could you also move `TestPassedException` inside `OverlappingTestBase` and make it `static`? This would resolve multiple definitions of `TestPassedException` when viewing source for AWT_Mixing in an IDE. test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 264: > 262: embedder.setPreferredSize(new Dimension(150, 150)); > 263: container.add(embedder); > 264: if(container instanceof Window){ Suggestion: if (container instanceof Window){ ------------- PR Review: https://git.openjdk.org/jdk/pull/25971#pullrequestreview-3184521964 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321616385 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321595814 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321636770 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321642154 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321646046 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321655342 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321652023 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321678210 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321679963 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321687965 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321703332 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321710616 PR Review Comment: https://git.openjdk.org/jdk/pull/25971#discussion_r2321711250
