<AWT Dev> [9] Review Request: 8004693 TEST_BUG: java/awt/KeyboardFocusmanager/DefaultPolicyChange/DefaultPolicyChange_Swing.java fails

[Semyon Sadetsky]

On 6/7/2016 11:48 AM, Sergey Bylokhov wrote:
> On 07.06.16 10:23, Semyon Sadetsky wrote:
>> Sergey,
>>
>> - You need to compare with the original policy by reference. Only by
>> that you may prove that the original policies were not affected.
>
> I do not see the reason why the equal cannot be used, if some policy 
> will override equal and return true then two policies will be treated 
> as equal/unchanged.
>
I cannot agree with this assumption. Since the policy should not be 
touched for the existing components, the test should prove that the 
policy object instances are the same, other behavior is an error.

>>
>> - Please remove the printouts of policies objects before and after the
>> default policy change. It looks too verbose.
>
> It was there before, and I found it quite useful during the fix.
And how is it useful? The toString() method is not implemented for the 
printed objects, so the printouts are not readable. I found this print 
out as excessive. If it invites to compare the memory addresses, that 
has no sense, because it is automatically executed at the end of the 
test and the same printout is produced in case of error.

>
>>
>> - It would be nice to have a check if the default policy has been really
>> changed to the new one.
>
> I did not get it, the bug which was bound to this test is about the 
> incorrect changing the policy by setDefaultFocusTraversalPolicy(). the 
> tests which cover other behavior are bound to other bugs like JDK-7125044
In this case, please, provide a link to the test which covers the 
KFM#setDefaultFocusTraversalPolicy() method.

>
>>
>> - What is purpose of jb1, jt1, jp1? They are created but never used.
>
> They are used as dummy content for JFrame jf.
That is what I asked. For what purpose this dummy content is added? Can 
you clarify that?

--Semyon
>
>> On 5/30/2016 7:39 PM, Sergey Bylokhov wrote:
>>> Hello.
>>> Please review the fix for jdk9.
>>>
>>> The test DefaultPolicyChange_Swing.java has two issues:
>>>  - It uses invokeLater(), so the test usually pass before the code is
>>> executed on the EDT, because the main thread completes before.
>>>  - The test fetches the FocusTraversalPolicy from the current
>>> KeyboardFocusManager. But default FocusTraversalPolicy can be changed
>>> during the Swing initialization(JDK-7125044). The test should save the
>>> state before setDefaultFocusTraversalPolicy() but after the Swing
>>> initialization, and validate that the FocusTraversalPolicy was not
>>> changed for windows which were already shown.
>>>
>>> The fix proposed in the CR is applied + small cleanup(regtesthelpers
>>> removed and InvokeAndWait is used instead of InvokeLater+realSync)
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8004693
>>> Webrev can be found at:
>>> http://cr.openjdk.java.net/~serb/8004693/webrev.01
>>>
>>
>
>