On Tue, 12 Mar 2024 09:00:18 GMT, Alexander Zuev <kiz...@openjdk.org> wrote:
>> test/jdk/javax/swing/JDesktopPane/4773378/bug4773378.java line 69:
>>
>>> 67: keyTyped = true;
>>> 68: bug4773378.this.notifyAll();
>>> 69: }
>>
>> A `CountDownLatch` would work great in this case.
>>
>> And `keyTyped` is a confusing name for a flag which gets to `true` when the
>> internal frame is activated.
>
> Renamed as frameActivated
I'm still for using `CountDownLatch`, it looks cleaner in my opinion.
>> test/jdk/javax/swing/JEditorPane/4325606/bug4325606.java line 115:
>>
>>> 113: try {
>>> 114: SwingUtilities.invokeAndWait(b::setupGUI);
>>> 115: safeSleep(3000);
>>
>> Instead of sleeping, you can use a `CountDownLatch(3)` which you'll
>> `countDown()` for each mouse click. Here you'll call `await(3,
>> TimeUnit.SECONDS)` and throw a timeout error if `await` returns `false`.
>>
>> Another synchroniser may be used to handle the case where
>> `BadLocationException` is thrown to fail the test right away. Alternatively,
>> `BadLocationException` may be wrapped into `RuntimeException` and re-thrown.
>
> This is a very simple case and sleeping worked for it so i do not see reason
> to rewrite it with CDL.
Using CDL can reduce the execution time… likely the three clicks are handled
more quickly than in 3 seconds.
>> test/jdk/javax/swing/JEditorPane/4694598/FrameContent.html line 1:
>>
>>> 1: <HTML><BODY>
>>
>> Can this file be created by the test on the fly? There's no need for a
>> supporting file.
>>
>> In #18147, Prasanta handled the same situation.
>
> I prefer to keep things simpler and not to create an additional possible
> point of failure. The supporting html file is self-explanatory and, if
> needed, can be changed outside of fixing the test itself.
It will make the test self-contained, which, in my opinion, is preferred.
>> test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java line 29:
>>
>>> 27: * @summary JEditor pane throws NullPointerException on mouse movement.
>>> 28: * @library ../../regtesthelpers
>>> 29: * @build JRobot
>>
>> The test doesn't seem to use any of the extended functionality provided by
>> JRobot, so the standard `java.awt.Robot` can be used instead.
>
> I do not see any harm in using JRobot, it is a Swing test after all.
No harm but a dependency on a library which isn't used.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521350860
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521346223
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521347941
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521348935
