Hi Prem,

Looks good.
Thanks!

On 10.03.2016 13:57, Prem Balakrishnan wrote:

Hi Alexey,

Updated the test as per review comments.

http://cr.openjdk.java.net/~arapte/prem/7012008/webrev.02/ <http://cr.openjdk.java.net/%7Earapte/prem/7012008/webrev.02/>

*JTreg @test tags:*

As per TAG SYNTAX, Test tags must be enclosed in a comment at the head of the file, refer below link

*http://openjdk.java.net/jtreg/tag-spec.html#TAG_SYNTAX , *

and we are following the same conventions in all the JDK9 tests.

I see. I remember I saw some tests had this JTreg tag comments before the class, and I find it more convenient because IDE doesn't collapse the comment automatically.

*Just to confirm: Does the test pass on Linux or Mac?*

*@requires (os.family == "windows") is added to tag, *

which doesn’t allow the following test to run on any other platform except windows.

(When executed using JTreg)

Thanks! I haven't seen that notation before, I just wanted to be sure.


Regards,
Alexey

Regards,
Prem

*From:*Alexey Ivanov
*Sent:* Thursday, March 10, 2016 3:07 PM
*To:* Prem Balakrishnan; Semyon Sadetsky; Rajeev Chamyal; Sergey Bylokhov; swing-dev@openjdk.java.net *Subject:* Re: <Swing Dev> Review Request: JDK-7012008 JDesktopPane - Wrong background color with Win7+WindowsLnf

Hi Prem,

Thank you for updating the test, it looks much simpler now.

Could you please move the comment with JTreg @test tags to the class declaration? I mean put it below imports.

The array of look-and-feels to test would be better readable if formatted like this:

        String[] lookAndFeel = new String[] {
"com.sun.java.swing.plaf.windows.WindowsLookAndFeel",
"com.sun.java.swing.plaf.windows.WindowsClassicLookAndFeel"
        };

Don't you agree?

I'm sure the line
        desktopPane.setVisible(true)
is redundant in this case.

Why do you catch the four exceptions in main? I see no reason to catch any exceptions in main: an exception will fail the test and you will see the real problem rather than the generic message "Setting WindowsLookAndFeel Failed".


Just to confirm: Does the test pass on Linux or Mac?

I also suggest renaming 'lookAndFeel1' variable in loop to 'laf'.

Regards,
Alexey

On 10.03.2016 9:45, Prem Balakrishnan wrote:

    Hi Semyon and Alexey,

    Thankyou for the Review.

    I have updated the test as per review comments.

    http://cr.openjdk.java.net/~arapte/prem/7012008/webrev.01/
    <http://cr.openjdk.java.net/%7Earapte/prem/7012008/webrev.01/>

    Regards,
    Prem

    *From:*Alexey Ivanov
    *Sent:* Wednesday, March 09, 2016 3:58 PM
    *To:* Prem Balakrishnan; Rajeev Chamyal; Sergey Bylokhov; Semyon
    Sadetsky; swing-dev@openjdk.java.net
    <mailto:swing-dev@openjdk.java.net>
    *Subject:* Re: <Swing Dev> Review Request: JDK-7012008
    JDesktopPane - Wrong background color with Win7+WindowsLnf

    Hi Prem,

    The fix itself looks good.
    Yet I have some questions to the test.

    Could you please place the comment with @test tag and other before
    the class declaration rather than before imports?
    Could you place asterisk in front of each line in this comment
    block? It will look consistent with Java style doc comments and
    other tests.

    Do you really need to create UI?
    Wouldn't it be enough to create JDesktopPane and check its
    background color? I believe it will significantly simplify the test.

    I think converting Color to ColorUIResource is unnecessary here.
    ColorUIResource is Color decorated with UIResource interface:
    Color.equals method does not take this distinction into account.

    In main(), you can create and initialize the LaF array in one go.

    In executeTest(), you should dispose mainFrame on EDT rather than
    on main thread. I'd recommend using finally block in run() to
    dispose mainFrame: it will handle both success and failure.

    Additionally, you access variables from different threads. The way
    the code is written now does not guarantee memory consistency
    between the different threads, thus the test could fail or pass
    sporadically.

    And I agree with Semyon, the test should fail on other platforms,
    you should just skip the test.


    Regards,
    Alexey

    On 09.03.2016 9:59, Prem Balakrishnan wrote:

        Hi*,*

        Please review fix for JDK9,

        *Bug:*https://bugs.openjdk.java.net/browse/JDK-7012008**

        *Webrev:*http://cr.openjdk.java.net/~arapte/prem/7012008/webrev.00/
        <http://cr.openjdk.java.net/%7Earapte/prem/7012008/webrev.00/>

        *Issue:*

        JDesktopPane - Wrong background color with Win7+WindowsLnf

        *Cause:*

        Desktop.background property set to "win.desktop.backgroundColor"

        *Fix:*

        Changed Desktop.background property  to "win.mdi.backgroundColor"

        Regards,
        Prem


Reply via email to