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

[Tejesh R]

On Fri, 19 May 2023 19:22:05 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Updated based on review comments
>
> 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.

Looks like its old code.

> 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.

Updated.

> 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);

Updated.

> 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()`.

Updated.

> 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.

Updated.

> 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`.

Updated.

> 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.

Updated.

> 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.

Updated.

> 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.

Updated.

> test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 114:
> 
>> 112:     }
>> 113: 
>> 114:     private static class Semaphore {
> 
> The `Semaphore` class is unused now, remove it.

Removed.

> 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`.

Updated.

> 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.

Updated.

> 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.

My bad, I updated the wrong commit for this particular file. Updated.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200041902
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042110
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042538
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200044118
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042017
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042228
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042318
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042657
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200043142
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200043909
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042837
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200042911
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200043736