RFR: 6850365: java/awt/Focus/WindowActivationFocusTest.java fails [v3]

[Alexey Ivanov]

On Thu, 18 Sep 2025 05:12:27 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

>> Test is made more robust by executing in EDT and cleanup done by frame dispose. Testing result is green.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Rename test

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/Focus/WindowActivationFocusTest.java line 1:

> 1: /*

// A very simple lightweight component
    class LightWeight extends Component implements FocusListener {

An AWT component is not lightweight but *heavyweight* as opposed to Swing component which are lightweight.

test/jdk/java/awt/Focus/WindowActivationFocusTest.java line 47:

> 45: 
> 46:     private static final int NUM_FRAMES = 2;
> 47:     private static ActivateFocus[] af;

You can give a meaningful name to `af`. These are frames, right?

The array itself could be initialised / created here.

test/jdk/java/awt/Focus/WindowActivationFocusTest.java line 52:

> 50:         try {
> 51:             WindowActivationFocusTest app = new WindowActivationFocusTest();
> 52:             EventQueue.invokeAndWait(() -> app.doTest());

Is it essential? The spec doesn't require `Frame` objects are created on EDT, shouldn't the test pass if the frames are created on the main thread?

test/jdk/java/awt/Focus/WindowActivationFocusTest.java line 57:

> 55:             boolean testFailed = false;
> 56:             for (int i = 0; i < NUM_FRAMES; i++) {
> 57:                 testFailed |= (af[i].lw.focusCounter > 1);

Accessing the `focusCounter` field is not thread-safe, it's value is updated on EDT but you read it on the main thread without any synchronisation. There are no guarantees if the main thread sees the updated value.

test/jdk/java/awt/Focus/WindowActivationFocusTest.java line 57:

> 55:             boolean testFailed = false;
> 56:             for (int i = 0; i < NUM_FRAMES; i++) {
> 57:                 testFailed |= (af[i].lw.focusCounter > 1);

Should the test fail if `af[i].lw.focusCounter` doesn't change its value from 0? I think it should, therefore the condition should be update to
Suggestion:

            for (int i = 0; i < NUM_FRAMES; i++) {
                testFailed |= (af[i].lw.focusCounter != 1);

test/jdk/java/awt/Focus/WindowActivationFocusTest.java line 77:

> 75:       af = new ActivateFocus[NUM_FRAMES];
> 76:       Dimension scrSize = Toolkit.getDefaultToolkit().getScreenSize();
> 77:       for (int i = 0; i < NUM_FRAMES; i++) {

Using the standard 4-space indentation in `doTest` would be really good.

test/jdk/java/awt/Focus/WindowActivationFocusTest.java line 79:

> 77:       for (int i = 0; i < NUM_FRAMES; i++) {
> 78:           af[i] = new ActivateFocus(i);
> 79:           af[i].setLocation(i * 160 + scrSize.width / 2, scrSize.height / 2);

Suggestion:

          af[i].setLocation(i * 160 + scrSize.width / NUM_FRAMES, scrSize.height / NUM_FRAMES);

Should this be updated to use the constant?

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

PR Review: https://git.openjdk.org/jdk/pull/27214#pullrequestreview-3254077563
PR Review Comment: https://git.openjdk.org/jdk/pull/27214#discussion_r2369458751
PR Review Comment: https://git.openjdk.org/jdk/pull/27214#discussion_r2369438407
PR Review Comment: https://git.openjdk.org/jdk/pull/27214#discussion_r2369438651
PR Review Comment: https://git.openjdk.org/jdk/pull/27214#discussion_r2369478108
PR Review Comment: https://git.openjdk.org/jdk/pull/27214#discussion_r2369483215
PR Review Comment: https://git.openjdk.org/jdk/pull/27214#discussion_r2369464455
PR Review Comment: https://git.openjdk.org/jdk/pull/27214#discussion_r2369468275