On Wed, 26 Apr 2023 14:22:52 GMT, Alexey Ivanov <aiva...@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
