On Tue, 25 Apr 2023 15:43:18 GMT, Tejesh R <t...@openjdk.org> wrote:
>> Open source few AWT ScrollPane related tests.
>
> Tejesh R has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Updated based on Review Comments
> - Updated based on Review Comments
test/jdk/java/awt/ScrollPane/ComponentScrollTest.java line 48:
> 46: public Frame frame;
> 47: public int count = 0;
> 48: public static void main(String[] args) throws InterruptedException,
> InvocationTargetException {
Suggestion:
public int count = 0;
public static void main(String[] args)
throws InterruptedException, InvocationTargetException {
Add a blank line between fields and method, wrap `throws` clause to fit into
80-column limit.
test/jdk/java/awt/ScrollPane/ComponentScrollTest.java line 68:
> 66: frame.add(scrollpane);
> 67: });
> 68: scrollpane.getVAdjustable().addAdjustmentListener(this);
The lambda expression has incorrect indentation.
You should add the listener on EDT too.
test/jdk/java/awt/ScrollPane/ScrollPaneExtraScrollBar.java line 31:
> 29: shown for the first time with SCROLLBARS_AS_NEEDED style
> 30: @key headful
> 31: */
In majority of cases, the tags have a leading asterisk.
test/jdk/java/awt/ScrollPane/ScrollPaneExtraScrollBar.java line 57:
> 55: robot = new Robot();
> 56: EventQueue.invokeAndWait(() -> {
> 57: f = new Frame();
Suggestion:
f = new Frame("ScrollPaneExtraScrollBar");
As suggested in other reviews, let's add a title to the frame with the name of
the test.
test/jdk/java/awt/ScrollPane/ScrollPaneExtraScrollBar.java line 71:
> 69: robot.delay(100);
> 70: robot.waitForIdle();
> 71: Rectangle r = f.getBounds();
You may want to call `getBounds()` on EDT.
test/jdk/java/awt/ScrollPane/ScrollPaneExtraScrollBar.java line 77:
> 75: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 76:
> 77: Thread.sleep(1000);
What you want here is
Suggestion:
Robot.waitForIdle();
test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 80:
> 78: Properties prop = System.getProperties();
> 79: String os = prop.getProperty("os.name", "");
> 80: if (os != null && (os.indexOf("98") == -1)) {
Shall we drop the filter for Windows 98? Java cannot be run on Windows 9x for
quite a long time, so it doesn't make sense to preserve this condition.
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 95:
> 93: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 94:
> 95: // Toolkit.getDefaultToolkit().sync();
Instead of commenting out the line, remove it.
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 120:
> 118: private static class Semaphore {
> 119: volatile boolean state = false;
> 120: Object lock = new Object();
Since it's a lock object, it should be `final`.
test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 121:
> 119: volatile boolean state = false;
> 120: Object lock = new Object();
> 121: volatile int waiting = 0;
Why are both `state` and `waiting` marked as volatile? Either is accessed from
synchronized blocks protected by `lock` except for `getState` which should also
use the `lock` object.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177959567
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177961343
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177957536
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177921389
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177922967
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177924261
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177938458
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177939984
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177944902
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1177950769
