On Thu, 25 Sep 2025 14:37:52 GMT, Khalid Boulanouare <[email protected]> wrote:
>> This PR will consolidate fixes of the following bugs: >> >> https://bugs.openjdk.org/browse/JDK-8361188 >> https://bugs.openjdk.org/browse/JDK-8361189 >> https://bugs.openjdk.org/browse/JDK-8361190 >> https://bugs.openjdk.org/browse/JDK-8361191 >> https://bugs.openjdk.org/browse/JDK-8361192 >> https://bugs.openjdk.org/browse/JDK-8361193 >> https://bugs.openjdk.org/browse/JDK-8361195 >> >> This PR depends on https://github.com/openjdk/jdk/pull/25971 >> >> For test : java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java, the fix >> suggested is to return false in method isValidForPixelCheck for embedded >> frame, in which case the component is set to null. For more details see bug: >> [JDK-8361188](https://bugs.openjdk.org/browse/JDK-8361188) >> >> For test : test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java, I >> had to create a a tolerance color matching method for mac for the tests to >> pass. Also, the jbuttons needed to have different color than the color of >> the background frame, in order for test to pass. For more detail see bug: >> https://bugs.openjdk.org/browse/JDK-8361193 >> >> For test : test/jdk/java/awt/Mixing/AWT_Mixing/JSplitPaneOverlapping.java, >> it seems that color selected for lightweight component matches the >> background color of the frame. And this will cause the test to fail when >> matching colors. Choosing any color different than the background color will >> get the test to pass. For more details, see bug: >> https://bugs.openjdk.org/browse/JDK-8361192 >> >> For test test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java, it >> looks like the frame when visible, the popup test does not work properly. >> The frame needs to be hidden for the test to click on popup. For more >> details see bug: https://bugs.openjdk.org/browse/JDK-8361191 >> >> For test test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java, the >> test runs successfully but it times out after the default 2 minutes of >> jtreg. increasing the timeout to 3 minutes get the test to pass. For more >> details please refer to bug: https://bugs.openjdk.org/browse/JDK-8361190 > > Khalid Boulanouare has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains 37 commits: > > - Merge branch 'pr/25971' into jdk-8360498 > - Merge branch 'openjdk:master' into jdk-8360498 > - Resolves confict for when there is a merge with jdk-8158801 > - Merge branch 'openjdk:master' into jdk-8360498 > - Removes not needed changes > - Revert "Removes not needed changes" > > This reverts commit e76780d50cc390e35443dccb193cfbc9a1cec1cb. > - Removes not needed changes > - Removes extra white lines > - Merge branch 'pr/25971' into jdk-8360498 > - Merge branch 'pr/25971' into jdk-8360498 > - ... and 27 more: https://git.openjdk.org/jdk/compare/59a937ac...900a7943 Changes requested by aivanov (Reviewer). test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 45: > 43: > 44: > 45: import javax.swing.JFrame; Imports look inconsistent. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 135: > 133: }); > 134: if (testResize) { > 135: wasLWClicked = false; Suggestion: if (!testResize) { return true; Invert the condition. We did this in #25971, it removes additional indentation and makes the code clearer. You probably want to move this condition above the focus latch. test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 157: > 155: } catch (InvocationTargetException ex) { > 156: fail(ex.getMessage()); > 157: } Suggestion: } catch (InterruptedException | InvocationTargetException ex) { fail(ex.getMessage()); } Merge them together. test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 71: > 69: > 70: > 71: cb = new JComboBox(petStrings); Suggestion: frame.setVisible(true); cb = new JComboBox(petStrings); One blank between code sections is enough. test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 77: > 75: frame.setSize(200, 200); > 76: frame.setLocationRelativeTo(null); > 77: frame.setVisible(true); There are calls to `setLocationRelativeTo(null)` and `setVisible(true)` in the bottom of the method. test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 80: > 78: lwClicked = true; > 79: frame.setVisible(false); > 80: } Why is it needed to hide the frame now? It wasn't hidden previously. test/jdk/java/awt/Mixing/AWT_Mixing/JSplitPaneOverlapping.java line 74: > 72: sp1 = new JScrollPane(p); > 73: currentAwtControl.setForeground(Color.white); > 74: currentAwtControl.setBackground(Color.white); Suggestion: currentAwtControl.setForeground(Color.WHITE); currentAwtControl.setBackground(Color.WHITE); Let's use upper-case *constants*. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 82: > 80: private static int frameBorderCounter() { > 81: > 82: String JAVA_HOME = System.getProperty("java.home"); Please don't add redundant blank lines at the very start of a method. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 85: > 83: try { > 84: Process p = Runtime.getRuntime() > 85: .exec(JAVA_HOME + "/bin/java FrameBorderCounter"); Did I already asked this question? Why can't the code in `FrameBorderCounter` be run in the same JVM? I'm pretty sure it can be. It's like for a separate refactoring, yet I'd like to have a tracking bug for getting rid of launching another process to count something that can be done by executing `FrameBorderCounter.main` directly and ensuring the frame is disposed of. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 106: > 104: private static String readInputStream(InputStream is) throws > IOException { > 105: > 106: byte[] buffer = new byte[4096]; Remove this redundant line. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 122: > 120: borderShift = frameBorderCounter(); > 121: borderShift = > 122: Math.abs(borderShift) == 1 ? borderShift : (borderShift > / 2); I'd leave it as is, or, if you prefer updating, rather reformat it like this: Suggestion: borderShift = Math.abs(borderShift) == 1 ? borderShift : (borderShift / 2); test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 171: > 169: public void run() { > 170: > 171: lLoc = frame.getLocationOnScreen(); Remove this redundant blank line… However, many `Runnable`s have a blank line right after the `run()` method declaration. In this case, I'd follow this style for consistency. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 185: > 183: public void run() { > 184: > 185: Point btnLoc = jbutton.getLocationOnScreen(); My preference would be to have only one of these blank lines. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 257: > 255: public static void main(String args[]) throws Exception { > 256: > 257: try { Suggestion: public static void main(String args[]) throws Exception { try { A blank line at the start of a method is redundant. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 311: > 309: public static synchronized void pass() { > 310: > 311: System.out.println("The test passed."); All these added blank lines are redundant. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 349: > 347: > 348: System.out.println("Comparing color: " + color + " with > reference " + > 349: "color: " + refColor); Suggestion: private static boolean isAlmostEqualColor(Color color, Color refColor) { System.out.println("Comparing color: " + color + " with reference " + "color: " + refColor); test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 356: > 354: TOLERANCE_MACOSX && > 355: Math.abs(color.getBlue() - refColor.getBlue()) < > 356: TOLERANCE_MACOSX; Suggestion: return color.equals(refColor) || (Math.abs(color.getRed() - refColor.getRed()) < TOLERANCE_MACOSX && Math.abs(color.getGreen() - refColor.getGreen()) < TOLERANCE_MACOSX && Math.abs(color.getBlue() - refColor.getBlue()) < TOLERANCE_MACOSX); I strongly prefer this style of wrapping: wrapping before a binary operator makes it clear that it's a continuation; parentheses explicitly group comparison of color components. I would not wrap the lines with `< TOLERANCE_MACOSX` although they don't fit 80-column limit. Wrapping doesn't help parsing the expression. You can shorten `TOLERANCE_MACOSX` to `TOLERANCE`. Yet the current name suggests the tolerance is in effect only on macOS. test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 361: > 359: static class TestPassedException extends RuntimeException { > 360: > 361: } I see no reason to add a blank line inside the declaration of the `TestPassedException` class. test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 387: > 385: return !(component == null || > 386: (component instanceof java.awt.Scrollbar) || > 387: (isMac && (component instanceof java.awt.Button))); Suggestion: return !(component instanceof java.awt.Scrollbar) && (!isMac || (!(component instanceof java.awt.Button))); The expression can be simplified further. `java.awt.Scrollbar` is already imported, so `Scrollbar` can safely be used. I'm for importing `java.awt.Button` and using unqualified `Button` in the expression. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 171: > 169: } else { > 170: latch.countDown(); > 171: } `latch` isn't declared in this scope. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 173: > 171: } > 172: try { > 173: boolean await = latch.await(1, TimeUnit.SECONDS); `latch` isn't declared in this scope. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 187: > 185: throw new RuntimeException( > 186: "Ancestor frame didn't receive focus"); > 187: } Indentation is wrong. test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 199: > 197: > 198: > 199: return wasLWClicked; One blank line seems enough between code sections. ------------- PR Review: https://git.openjdk.org/jdk/pull/26625#pullrequestreview-3569053278 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611877977 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611867427 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611872778 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611876184 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611881799 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611885553 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611903091 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611909297 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2612020583 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611919078 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611924803 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611929707 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611933903 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611940143 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611942003 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611943663 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611965281 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611945982 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611979119 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611999199 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2612003424 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611987770 PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2612005602
