Looks good to me.
Regards, Rajeev Chamyal From: Avik Niyogi Sent: 19 July 2016 12:12 To: Alexandr Scherbatiy Cc: Rajeev Chamyal; Semyon Sadetsky; Yuri Nesterenko; swing-dev@openjdk.java.net Subject: Re: <Swing Dev> 8160438: [PIT][macosx] [TEST_BUG] javax/swing/plaf/nimbus/8057791/bug8057791.java fails Hi All, Another gentle reminder. Please review the changes made. On 15-Jul-2016, at 2:50 pm, Avik Niyogi <HYPERLINK "mailto:avik.niy...@oracle.com"avik.niy...@oracle.com> wrote: A gentle reminder, please review the changes On 14-Jul-2016, at 8:46 pm, Alexandr Scherbatiy <HYPERLINK "mailto:alexandr.scherba...@oracle.com"alexandr.scherba...@oracle.com> wrote: The fix looks good to me. Thanks, Alexandr. On 7/14/2016 9:53 AM, Avik Niyogi wrote: Hi All, Please find my webrev below for review which includes changes as perceived from inputs provided. HYPERLINK "http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.02/"http://cr.openjdk.java.net/~aniyogi/8160438/webrev.02/ With Regards, Avik Niyogi On 12-Jul-2016, at 7:12 pm, Alexandr Scherbatiy <HYPERLINK "mailto:alexandr.scherba...@oracle.com"alexandr.scherba...@oracle.com> wrote: On 7/12/2016 8:24 AM, Avik Niyogi wrote: Hi All, A gentle reminder, please review my code changes. With Regards, Avik Niyogi On 08-Jul-2016, at 4:24 pm, Yuri Nesterenko <HYPERLINK "mailto:yuri.nestere...@oracle.com"yuri.nestere...@oracle.com> wrote: It pass for me if I provide some time to process native events after the user activity simulation. For instance, setAutoDelay(50) for robot does that plus waitForIdle() after every mouseMove(). In this case, the part with that additional check and a (misleading, I think) comment about the color profiles may be removed. The test would take much more time though, and @run main/timeout=600 bug8057791 line would be necessary. Some more comments to the previous ones: - runColorTestCase() uses JList and should be run on EDT - "if (tryNimbusLookAndFeel()) {" It is supposed that the Nimbus L&F is supported on all platforms. May be it is better to fail the test if the Nimbus L&F is not set. - "if (osName.contains("Mac")) { isMac = true; }" can be simplified to "return osName.contains("Mac")" - " if (!"".equals(errorString)) {" can be simplified to !errorString.isEmpty() Thanks, Alexandr. Thanks, -yan On 07/08/2016 04:28 AM, Avik Niyogi wrote: The test does not pass if mac specific check is not done for backgroundcolor. The check is required to get the same values from BufferedImage as they are the same as found with Digital Color Meter. This test case fixes that. Please provide inputs if this fix will get a +1 or if not any positive inputs to modify the test case will be welcome. With Regards, Avik Niyogi On 07-Jul-2016, at 9:51 pm, Semyon Sadetsky <HYPERLINK "mailto:semyon.sadet...@oracle.com"semyon.sadet...@oracle.com <mailto:semyon.sadet...@oracle.com>> wrote: On 7/7/2016 6:30 PM, Yuri Nesterenko wrote: On 07/07/2016 06:15 PM, Semyon Sadetsky wrote: On 7/7/2016 5:58 PM, Yuri Nesterenko wrote: On 07/07/2016 05:35 PM, Yuri Nesterenko wrote: On 07/07/2016 05:04 PM, Semyon Sadetsky wrote: On 07.07.2016 16:35, Avik Niyogi wrote: Hi Semyon, Thank you for the review comment. In Mac OS X, *System Preferences > Displays > Colors > Display Profile* section, the default value is *Color LCD*. This causes a failure in some test cases which uses robot.The colour configuration it expects to use is the *Generic RGB Profile*. That is what "Non-generic display settings" means. AFAIK there are instruction that tests should be executed using color profile with no color corrections on OS X. I guess there are many other tests that fail with color correction. If this is a root cause than the bug doesn't need to be fixed. Well, I filed this bug and I used Generic RGB on all my test machines. There may be additional precautions in the tests about that but misconfiguration is not the root case here. That said, I feel vary about attempts to guess Apple colors wary I mean in non-generic profiles. Yuri. Do you mean that the non-generic is not a root case? No: I had Generic RGB everywhere, and it failed for me. I should say that the new version of the test properly fails with b120 and pass with current PIT. And colors are visibly different in the two builds: so the test works OK now. That contradicts with the description of the fix. Probably the test does unnecessary care about the non-Generic profiles. 159 if (!foundMatch && isMac()) { 160 //One more chance for Mac due to non-Generic display setting 161 detectedColor = new Color(img.getRGB(x, y), true); 162 if (detectedColor.equals(colorCheck)) { 163 foundMatch = true; 164 break checkLoops; 165 } 166 } Does it mean that the code above, which is necessary to serve non-Generic profiles on OS X, may be removed and the test still passes? --Semyon -yan --Semyon -yan --Semyon Regarding "Negative scenarios", these include cases where colours are different from the expected background or foreground colors is checked with the robot and BufferedImage respectively to *eliminate false positives due to coincidentally finding a match* by using robot or BufferedImage. Please find my changes appropriating the inputs provided. I removed the variable foundMatch as I found that it is not required at all and incorporated the suggestion to use return instead of a labelled break. HYPERLINK "http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.01/"http://cr.openjdk.java.net/~aniyogi/8160438/webrev.01/ HYPERLINK "http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.01/";<http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.01/> On 07-Jul-2016, at 6:30 pm, Semyon Sadetsky <HYPERLINK "mailto:semyon.sadet...@oracle.com"semyon.sadet...@oracle.com HYPERLINK "mailto:semyon.sadet...@oracle.com";<mailto:semyon.sadet...@oracle.com>> wrote: Hi Avik, could you clarify what is "Non-generic display settings"? Is it known bug on OS X? And also please be more specific on "negative scenarios" why they are necessary ? Also could you replace labeled break with "return foundMatch; " --Semyon On 07.07.2016 15:11, Avik Niyogi wrote: Hi All, Kindly review the fix for JDK9. *Bug: *HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8160438";<https://bugs.openjdk.java.net/browse/JDK-8160438>https://bugs.openjdk.java.net/browse/JDK-8160438 *Webrev: *HYPERLINK "http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.00/";<http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.00/>http://cr.openjdk.java.net/~aniyogi/8160438/webrev.00/ *Issue: *test javax/swing/plaf/nimbus/8057791/bug8057791.java consistently fails on OS X 10.10 *Cause: * Due to bug in detecting color for Non-generic display settings for Mac hardware, test case failed *Fix: *Positive and negative scenarios was added to check the colour for the Nimbus LAF foreground and background colours. With Regards, Avik Niyogi
