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

[Alexey Ivanov]

On Mon, 22 May 2023 07:06:37 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/ScrollPaneExtraScrollBar.java line 71:

> 69:         try {
> 70:             robot.delay(100);
> 71:             robot.waitForIdle();

Optionally,
Suggestion:

            robot = new Robot();
            robot.waitForIdle();
            robot.delay(100);

To be consistent with other tests in this review.

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

> 113:                         Container cp = child.getParent();
> 114:                         p = cp.getLocation();
> 115:                         if (p.y != -SCROLL_POS) {

Suggestion:

                        p = cp.getLocation();
                        System.out.println("Child's parent pos = " + p);
                        if (p.y != -SCROLL_POS) {

Just to make it clear that this branch is taken. Otherwise the test ends with the message:

Child pos = java.awt.Point[x=0,y=0]

which is confusing. Yet no exception is thrown.

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

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

Suggestion:

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

There's no need to update the value stored in `p`.

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

> 74:             frame.pack();
> 75:             frame.setLocationRelativeTo(null);
> 76:             frame.setAlwaysOnTop(true);

I wonder why you decided to make the frame in this test always on top?

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

> 81:     public void start() throws Exception {
> 82:         try {
> 83:             Robot robot = new Robot();

Since you moved it as suggested, I see no reason why it's declared here rather than at line before `robot.waitForIdle`, that is the first usage.

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

> 83:                     System.out.println("Adjustment Vertical Scroll Event called ");
> 84:                 }
> 85:             });

Suggestion:

            vScroll.addAdjustmentListener(ScrollPaneWindowsTest.this);

Isn't it more concise without introducing any duplicate code?

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

> 113: 
> 114:             robot.delay(100);
> 115:             robot.waitForIdle();

Suggestion:

            robot = new Robot();
            robot.waitForIdle();
            robot.delay(100);

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

> 75:             Robot robot = new Robot();
> 76:             robot.delay(1000);
> 77:             robot.waitForIdle();

Suggestion:

            Robot robot = new Robot();
            robot.waitForIdle();
            robot.delay(1000);

Let's make this piece of code consistent in all the tests.

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

PR Review: https://git.openjdk.org/jdk/pull/13621#pullrequestreview-1436387950
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200456271
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200463397
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200362994
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200364050
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200366170
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200370354
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200468052
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1200373022