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

[Tejesh R]

On Wed, 26 Apr 2023 14:22:52 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

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

Updated.

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

Updated.

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

But I have been doing without leading asterisk for all the open sourcing, let me keep it this way.

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

Updated.

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

Updated.

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

Updated.

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

Yeah, we can drop it if its not supported.

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

Updated.

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

Updated.

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

I don't think its required to be volatile, not sure.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178146823
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178147063
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178146679
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178140844
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178141008
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178141210
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178141876
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178142070
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178142245
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1178145730