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

Tejesh R tr at openjdk.org
Wed May 3 04:37:31 UTC 2023


On Thu, 27 Apr 2023 12:13:57 GMT, Alexey Ivanov <aivanov at 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 47:
> 
>> 45:     public ScrollPane scrollpane;
>> 46:     public Frame frame;
>> 47:     public int count = 0;
> 
> Suggestion:
> 
>     public volatile int count = 0;
> 
> `count` is accessed from two threads, it should be `volatile` or you should use `AtomicInteger` instead.

Updated.

> test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 53:
> 
>> 51:     public static boolean mouseWasPressed = false;
>> 52:     public static Component child = null;
>> 53:     private Object lock = new Object();
> 
> Suggestion:
> 
>     private final Object lock = new Object();

Updated.

> test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 83:
> 
>> 81:                         if (e.getID() == MouseEvent.MOUSE_PRESSED) {
>> 82:                             if (e.getSource() == ScrollPaneLimitation.child
>> 83:                                     && e.getY() > SCROLL_POS) {
> 
> The two `if` blocks could be merged together into one.

Updated.

> test/jdk/java/awt/ScrollPane/ScrollPaneLimitation.java line 145:
> 
>> 143:                     throw new RuntimeException("test was interrupted");
>> 144:                 }
>> 145:             }
> 
> A `CountDownLatch` would be better rather than plain `lock`.

Updated.

> test/jdk/java/awt/ScrollPane/ScrollPaneRemoveAdd.java line 138:
> 
>> 136:                 waiting--;
>> 137:             }
>> 138:         }
> 
> This method is never used, it can safely be dropped.

Updated.

> test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest.java line 77:
> 
>> 75:         hScroll = (ScrollPaneAdjustable) sp.getHAdjustable();
>> 76:         vScroll.addAdjustmentListener(this);
>> 77:         hScroll.addAdjustmentListener(this);
> 
> `vScroll` and `hScroll` should also go into `invokeAndWait` above.

Updated.

> test/jdk/java/awt/ScrollPane/ScrollPositionIntact.java line 85:
> 
>> 83: 
>> 84:                 int i;
>> 85:                 i = (int) (sp.getScrollPosition().getX());
> 
> Suggestion:
> 
>                 int i = (int) (sp.getScrollPosition().getX());

Updated.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1183230264
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1183229539
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1183230178
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1183230201
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1183229514
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1183229628
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1183229572



More information about the client-libs-dev mailing list