On Tue, 16 May 2023 14:17:31 GMT, Tejesh R <t...@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
