RFR: 8306137: Open source several AWT ScrollPane related tests [v9]

[Alexey Ivanov]

On Tue, 16 May 2023 14:17:31 GMT, Tejesh R <tr at openjdk.org> wrote:

>> Open source few AWT ScrollPane related tests.
>
> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Updated based on review comments

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/ScrollPane/ComponentScrollTest.java line 82:

> 80:         try {
> 81:             EventQueue.invokeAndWait(() -> {
> 82:                 scrollpane.getVAdjustable().setValue(20);

I suggest moving the remaining line to `init()` too. The code will be cleaner then.

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 47:

> 45: import java.awt.event.MouseAdapter;
> 46: 
> 47: import java.util.Properties;

The `Properties` import is unused.

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 54:

> 52:     static final int SCROLL_POS = 50000;
> 53:     public static Component child = null;
> 54:     static CountDownLatch go = new CountDownLatch(1);

Suggestion:

    static final CountDownLatch go = new CountDownLatch(1);

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 90:

> 88:             pane.doLayout();
> 89:         });
> 90:         robot = new Robot();

It makes no difference, yet `robot` isn't used in `init()`, so I propose moving it to `start()`.

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 96:

> 94:         try {
> 95:             robot.delay(1000);
> 96:             robot.waitForIdle();

Suggestion:

            robot.waitForIdle();
            robot.delay(1000);

Usually, these lines come in reverse order.

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 128:

> 126:                 Dimension d = pane.getSize();
> 127:                 point.x += d.width / 2;
> 128:                 point.y += d.height / 2;

Suggestion:

                p = pane.getLocationOnScreen();
                Dimension d = pane.getSize();
                point = new Point(p.x += d.width / 2, p.y += d.height / 2);

This is safer, only one write to volatile `point`.

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 133:

> 131:             robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
> 132:             robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 133:             robot.waitForIdle();

This `waitForIdle` is redundant because you're waiting for the count-down on the next line.

test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 94:

> 92:             robot.waitForIdle();
> 93:             robot.mouseMove(buttonLoc.x + buttonSize.width / 2,
> 94:                     buttonLoc.y + buttonSize.height / 2);

Suggestion:

            robot.waitForIdle();
            robot.delay(1000);

            robot.mouseMove(buttonLoc.x + buttonSize.width / 2,
                    buttonLoc.y + buttonSize.height / 2);


I propose reversing `delay` and `waitForIdle` to the usual order. (Robot could be create here rather than in `init`.

I propose adding a blank line to visually separate waiting from the posting events to the UI.

test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 100:

> 98:             robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 99: 
> 100:             robot.delay(50);

This additional `delay` is redundant, you're waiting on the next line anyway.

test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 114:

> 112:     }
> 113: 
> 114:     private static class Semaphore {

The `Semaphore` class is unused now, remove it.

test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 87:

> 85: 
> 86:         vScroll.addAdjustmentListener(this);
> 87:         hScroll.addAdjustmentListener(this);

[In my previous comment](https://github.com/openjdk/jdk/pull/13621#discussion_r1179076672), I also meant listeners should also be added inside `invokeAndWait`.

test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 135:

> 133: 
> 134:         notifyReceived = false;
> 135:         synchronized (LOCK) {

Suggestion:

        synchronized (LOCK) {
            notifyReceived = false;

The `notifyReceived` must always be read and written inside a synchronised block. Other places should also be corrected.

test/jdk/java/awt/ScrollPane/ScrollPositionIntact.java line 72:

> 70:             });
> 71:         }
> 72:     }

It does not even compile.

Add a blank line before `init` and remove `try`-block to make it compilable.

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

PR Review: https://git.openjdk.org/jdk/pull/13621#pullrequestreview-1434978305
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199297623
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199304933
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199307181
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199301306
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199301712
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199306087
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199306770
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199312034
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199312394
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199312739
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199320045
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199325017
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1199330843