On Thu, 30 May 2024 19:24:32 GMT, Alisen Chung <ach...@openjdk.org> wrote:
>> Abhishek Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> background method removed
>
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 61:
>
>> 59: "com.sun.java.swing.plaf.gtk.GTKLookAndFeel");
>> 60: } catch (Exception e) {
>> 61: System.out.println("GTK LAF is not supported on this system;
>> test passes");
>
> should this be jtreg.testSkipped exception?
Updated.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 70:
>
>> 68: Class<? extends JTextComponent>
>> type)
>> 69: throws Throwable
>> 70: {
>
> I think formatting here looks a little odd, could probably move the bracket
> onto the same line as `throws Throwable` and shift it over to the right to
> match `Class<? extends JTextComponent> type)`
Updated.
> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 110:
>
>> 108: }
>> 109:
>> 110: private void onEDT10() {
>
> is this method required? can it be rewritten to not use onEDT10 and
> onBackgroundThread20?
It can be re-written without using `onEDT10` and `onBackgroundThread20`.. but I
kept it as it was in original test. Don't see any strong reason to change also
and there are many other tests which used `SwingTestHelper` utility to perform
the testing.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621720308
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621720428
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1621720186
