On Wed, 18 Aug 2021 21:56:51 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> lawrence.andrews has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fixed review comments
>
> test/jdk/java/awt/im/4959409/bug4959409.java line 24:
>
>> 22: */
>> 23:
>> 24: /**
>
> I suggest to remove the second `*` to avoid the warning from the IDE about
> the dangling Javadoc comment.
removed
> test/jdk/java/awt/im/4959409/bug4959409.java line 147:
>
>> 145:
>> 146: if (!keyPressedEventLatch.await(TIMEOUT, TimeUnit.SECONDS)) {
>> 147: throw new RuntimeException("Did not received KEY PRESS for
>> Shift + 1 , test failed");
>
> Suggestion:
>
> throw new RuntimeException("Did not receive KEY PRESS for Shift +
> 1 , test failed");
>
> And the same for “KEY PRESS”.
Done
> test/jdk/java/awt/im/4959409/bug4959409.java line 170:
>
>> 168: }
>> 169:
>> 170: public static boolean isTopLevelVisible(javax.swing.JFrame jFrame,
>> CountDownLatch topLevelVisibleLatch) throws Exception {
>
> The logic in this method polls `jFrame.isVisible` instead of waiting for it
> to become visible. Nothing wrong with this approach yet the last call
> `topLevelVisibleLatch.await(TIMEOUT, TimeUnit.SECONDS)` doesn't make sense in
> this case as the latch can't reach 0 if it's not zero.
>
> So, if something goes wrong, the test still waits for `TIMEOUT` (30) seconds
> for the condition that can never be satisfied. I suggest returning
> `topLevelVisibleLatch.getCount() == 0` and save 30 seconds.
>
> Probably it's enough to check whether the text field is visible. The text
> field can become visible only if frame is visible, both likely become visible
> at the same moment anyway.
Removed this method
> test/jdk/java/awt/im/4959409/bug4959409.java line 216:
>
>> 214: if (frame != null) {
>> 215: SwingUtilities.invokeAndWait(frame::dispose);
>> 216: }
>
> This has inconsistency where `frame != null` is run on main thread without
> synchronisation. The null check should also be run on the EDT as I originally
> suggested.
Done
-------------
PR: https://git.openjdk.java.net/jdk/pull/5058
