On Wed, 20 Sep 2023 16:55:30 GMT, Alexander Zvegintsev <azveg...@openjdk.org>
wrote:
>> Open sourcing few tests:
>>
>> java/awt/Frame/FrameRepackTest.java
>> java/awt/Frame/FrameResizeTest/FrameResizeTest_1.java
>> java/awt/Frame/FrameResizeTest/FrameResizeTest_2.java
>> java/awt/Frame/WindowMoveTest.java
>
> Alexander Zvegintsev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> space added
Do we add `@Override` annotations?
test/jdk/java/awt/Frame/FrameRepackTest.java line 53:
> 51: The menu has two items, labelled 'visible' and 'not visible'.
> 52: The frame also contains a red panel that contains two line
> labels,
> 53: ' This panel is always displayed' and ' it is a test'.
Suggestion:
'This panel is always displayed' and 'it is a test'.
test/jdk/java/awt/Frame/FrameRepackTest.java line 64:
> 62: You can repeatedly add and remove the second panel in this
> way.
> 63: After such an addition or removal, the frame's location on
> the screen
> 64: should not have changed, while the size changes to accommodate
Suggestion:
should not change, while the size changes to accommodate
It's happening now, during the test.
test/jdk/java/awt/Frame/FrameRepackTest.java line 116:
> 114: Menu flip = new Menu("Flip");
> 115: MenuItem mi;
> 116: flip.addSeparator();
The separator as the first item in a drop-down menu looks weird and confusing,
I propose removing it.
test/jdk/java/awt/Frame/FrameRepackTest.java line 134:
> 132: north.setLayout(new BorderLayout(2, 2));
> 133: north.add("North", new Label(" This panel is always displayed"));
> 134: north.add("Center", new Label(" it is a test "));
Suggestion:
north.add("North", new Label("This panel is always displayed"));
north.add("Center", new Label("it is a test "));
Is the space used to offset the text from the edge?
test/jdk/java/awt/Frame/FrameRepackTest.java line 169:
> 167: south.setVisible(true);
> 168: pack();
> 169: setVisible(true);
Nothing should change if you remove `setVisible(true)` here, the frame is never
hidden, is it?
test/jdk/java/awt/Frame/FrameResizeTest/FrameResizeTest_1.java line 45:
> 43: To the right of this frame is an all-white 200x200 frame.
> 44:
> 45: This is actually a white canvas component of the frame.
Suggestion:
This is actually a white canvas component in the frame.
test/jdk/java/awt/Frame/FrameResizeTest/FrameResizeTest_1.java line 48:
> 46: The frame itself is red.
> 47: The red should never show.
> 48: In particular, after you resize the frame, you should see all
> white and no red.
If I resize the frame quickly, I see some areas in red, I guess it's expected…
test/jdk/java/awt/Frame/FrameResizeTest/FrameResizeTest_1.java line 62:
> 60: .build();
> 61:
> 62: SwingUtilities.invokeAndWait(() -> {
Suggestion:
EventQueue.invokeAndWait(() -> {
Using `EventQueue` is more appropriate for AWT components.
test/jdk/java/awt/Frame/FrameResizeTest/FrameResizeTest_2.java line 95:
> 93:
> 94: Container dumbc = new DumbC();
> 95: add( dumbc, c );
Suggestion:
add(dumbc, c);
test/jdk/java/awt/Frame/FrameResizeTest/FrameResizeTest_2.java line 98:
> 96:
> 97: Panel dump = new DumbPanel();
> 98: add( dump, c );
Suggestion:
add(dump, c);
test/jdk/java/awt/Frame/FrameResizeTest/FrameResizeTest_2.java line 100:
> 98: add( dump, c );
> 99:
> 100: setSize( new Dimension( 300, 300));
Suggestion:
setSize(new Dimension(300, 300));
or directly `(300, 300)` without creating the `Dimension` object.
test/jdk/java/awt/Frame/WindowMoveTest.java line 56:
> 54: frame.dispatchEvent(new WindowEvent(frame,
> WindowEvent.WINDOW_CLOSING)));
> 55:
> 56: WindowMove.latch.await();
Add a timeout for `await` just in case? However, jtreg will also handle timeout.
test/jdk/java/awt/Frame/WindowMoveTest.java line 69:
> 67:
> 68: static CountDownLatch latch = new CountDownLatch(1);
> 69: static volatile String failMessage = null;
It does not need to be `volatile`, using the latch establishes the _happens
before_ relation between the EDT and the main thread.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15787#pullrequestreview-1642316127
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1335980344
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336031064
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336032622
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336035280
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336038451
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336042208
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336043985
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336045648
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336047171
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336047580
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336048137
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336060409
PR Review Comment: https://git.openjdk.org/jdk/pull/15787#discussion_r1336057460
