Hi All,
Another gentle reminder. Please review the changes made.
> On 15-Jul-2016, at 2:50 pm, Avik Niyogi <avik.niy...@oracle.com> wrote:
> 
> A gentle reminder, please review the changes
> 
>> On 14-Jul-2016, at 8:46 pm, Alexandr Scherbatiy 
>> <alexandr.scherba...@oracle.com <mailto: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.
>>> 
>>> http://cr.openjdk.java.net/~aniyogi/8160438/webrev.02/ 
>>> <http://cr.openjdk.java.net/%7Eaniyogi/8160438/webrev.02/>
>>> 
>>> With Regards,
>>> Avik Niyogi
>>>> On 12-Jul-2016, at 7:12 pm, Alexandr Scherbatiy 
>>>> <alexandr.scherba...@oracle.com <mailto: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 
>>>>>> <mailto: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> 
>>>>>>>> <mailto: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/>
>>>>>>>>>>>>>> <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
>>>>>>>>>>>>>>> <semyon.sadet...@oracle.com <mailto:semyon.sadet...@oracle.com> 
>>>>>>>>>>>>>>> <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:
>>>>>>>>>>>>>>>> *<https://bugs.openjdk.java.net/browse/JDK-8160438> 
>>>>>>>>>>>>>>>> <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:
>>>>>>>>>>>>>>>> *<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/
>>>>>>>>>>>>>>>>  <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
>>> 
>> 
> 

Reply via email to