On Fri, 19 May 2023 19:22:05 GMT, Alexey Ivanov <aiva...@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
