Hi All, Please find my webrev below for review which includes changes as perceived from inputs provided.
http://cr.openjdk.java.net/~aniyogi/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 > <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 <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 >>>>> <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. >>>>>>>>>>> >>>>>>>>>>> http://cr.openjdk.java.net/~aniyogi/8160438/webrev.01/ >>>>>>>>>>> <http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.01/> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> On 07-Jul-2016, at 6:30 pm, Semyon Sadetsky >>>>>>>>>>>> <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: >>>>>>>>>>>>> *<https://bugs.openjdk.java.net/browse/JDK-8160438>https://bugs.openjdk.java.net/browse/JDK-8160438 >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> *Webrev: >>>>>>>>>>>>> *<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
