<AWT Dev> [7] Review Request for 8047961: [TESTBUG] closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html fails intermittently

[mikhail cherkasov]

Hi Anton, Oleg.

I vote for removing the test.

Rewriting test just will produce something weired that won't test what 
original regression does.
A new test won't be a regression test that check problem was found in 
jdk 1.4.
I believe jdk4 and 7 has a lot of difference in focus system and due 
those changes the test became
irrelevant.

Thanks,
Mikhail.

On 11/17/2014 5:42 PM, Anton V. Tarasov wrote:
> On 17.11.2014 17:38, Oleg Sukhodolsky wrote:
>> On Mon, Nov 17, 2014 at 5:31 PM, Anton V. Tarasov
>> <anton.tarasov at oracle.com> wrote:
>>> On 17.11.2014 17:21, Oleg Sukhodolsky wrote:
>>>> On Mon, Nov 17, 2014 at 4:34 PM, Anton V. Tarasov
>>>> <anton.tarasov at oracle.com> wrote:
>>>>> On 17.11.2014 15:01, Oleg Sukhodolsky wrote:
>>>>>> Hi Anton,
>>>>>>
>>>>>> the bug was a regression introduced in 1.4 (comparing with 1.3.1) 
>>>>>> this
>>>>>> is why it was fixed and the test was written.
>>>>>> Indeed the spec doesn't guarantee that the test will work but at the
>>>>>> time we were working on the ticket it was decided that we should not
>>>>>> allow such regressions.
>>>>>> If (according to the current policy) the spec is more important than
>>>>>> compatibility in this case I'd suggest to remove the test completely
>>>>>> since its new version doesn't test for the regression we had earlier
>>>>>> but just test spec in a very strange/complicated way.
>>>>>
>>>>> The test passes on Windows, but fails on Mac and I suppose Linux. I
>>>>> recall I
>>>>> made attemps to fix the failure on Linux (in 1.6 or 1.7) but that was
>>>>> quite
>>>>> complicated to make something reliable, due to the fact that in 
>>>>> XToolkit
>>>>> even native focus is asynchronous, like in CToolkit. So, from my 
>>>>> point of
>>>>> view hacking it doesn't worth the candles, taking into account all 
>>>>> I've
>>>>> said
>>>>> below.
>>>>>
>>>>> I still like the idea of  replacing requestFocus with
>>>>> requestFocusInWindow.
>>>>> We can add comments saying that the original test was modified to 
>>>>> avoid
>>>>> the
>>>>> endless loop. It didn't actually test for the endless loop, as 
>>>>> otherwise
>>>>> it
>>>>> would have had a timer. It tested for focus gains count per 
>>>>> window, that
>>>>> remains relevant even in its new form. That's my personal opinion, I
>>>>> won't
>>>>> insist on keeping it if you both vote for the opposite...
>>>> if you do not plan to fix the problem the test reproduces then I'd
>>>> vote for removing the test (you have enough tests in regression tests
>>>> suite ;)
>>>
>>> I'm afraid I don't, as I don't think this is a real problem :)
>> so just remove the test, you do not need it ;)
>
> No, I won't. I'd leave it at Mikhail's discretion, as he is the author 
> of the request.
>
> Mikhail, you have a chance to save the test :)
>
> Regards,
> Anton.
>
>>
>> Oleg.
>>> Regards,
>>> Anton.
>>>
>>>
>>>> Regards, Oleg.
>>>>> Thanks,
>>>>> Anton.
>>>>>
>>>>>
>>>>>> So, it is you whole should make the final decision but I just do not
>>>>>> see a reason to keep a test which doesn't test for the particular
>>>>>> regression in regression tests suite.
>>>>>>
>>>>>> Regards, Oleg.
>>>>>>
>>>>>> On Mon, Nov 17, 2014 at 2:01 PM, Anton V. Tarasov
>>>>>> <anton.tarasov at oracle.com> wrote:
>>>>>>> Hi Oleg,
>>>>>>>
>>>>>>> Glad to hear from you :)
>>>>>>>
>>>>>>> On 14.11.2014 18:24, Oleg Sukhodolsky wrote:
>>>>>>>> Sorry to interrupt you but since I do know the test let me say 
>>>>>>>> that
>>>>>>>> requestFocus() is an important part of the test,
>>>>>>>> if you are going to replace it with requestFocusInWindow() you
>>>>>>>> (effectively) remove it from list of regression tests.
>>>>>>>> Please check 4369903 bug for more information.
>>>>>>>
>>>>>>> In the bug I see the description of why the infinite loop 
>>>>>>> happens and a
>>>>>>> suggestion to use requestFocusInWindow to prevent it. From my 
>>>>>>> point of
>>>>>>> view,
>>>>>>> the behavior is expected taking into account the asynchronous 
>>>>>>> nature of
>>>>>>> java
>>>>>>> focus.
>>>>>>>
>>>>>>> By default, when a toplevel window gains activation,
>>>>>>> KeyboardFocusManager
>>>>>>> requests focus in it by means of calling the requestFocusInWindow
>>>>>>> method.
>>>>>>> Thus, the test case creates an artificial situation with unclear 
>>>>>>> goals.
>>>>>>> It
>>>>>>> doesn't seem to reflect a real use case (I can see the bug was 
>>>>>>> filed by
>>>>>>> an
>>>>>>> internal engineer, not refering to any customer or user 
>>>>>>> application).
>>>>>>> In
>>>>>>> case a developer wants to alter the component receiving focus at 
>>>>>>> the
>>>>>>> toplevels's activation, he/she can achieve this by means of 
>>>>>>> customizing
>>>>>>> the
>>>>>>> focus traversal policy, for instance.
>>>>>>>
>>>>>>> The javadoc [1] for the JComponent.requestFocus method warns 
>>>>>>> developers
>>>>>>> about its usage:
>>>>>>>
>>>>>>> <<Note that the use of this method is discouraged because its 
>>>>>>> behavior
>>>>>>> is
>>>>>>> platform dependent. Instead we recommend the use of
>>>>>>> requestFocusInWindow().>>
>>>>>>>
>>>>>>> What I'm trying to say is that we'd rather avoid hacking the focus
>>>>>>> subsystem
>>>>>>> in order to just solve the infinite loop problem.
>>>>>>>
>>>>>>> However, originally the reporter of the bug complains about the
>>>>>>> following:
>>>>>>>
>>>>>>> <<Setting focus during window activation often sends focus to 
>>>>>>> the wrong
>>>>>>> place, or to multiple places.>>
>>>>>>>
>>>>>>> This behavior is incorrect, indeed. But we can verify it with 
>>>>>>> the test
>>>>>>> case.
>>>>>>> When a component gains focus we can check that:
>>>>>>>
>>>>>>> 1. it is the current focus owner reported by KFM, and the current
>>>>>>> focused
>>>>>>> window is its container
>>>>>>> 2. the opposite component has lost focus prior to that (that is, 
>>>>>>> every
>>>>>>> FOCUS_GAINED precedes a FOCUS_LOST received by the opposite 
>>>>>>> component)
>>>>>>> 3. the same about the WINDOW_LOST_FOCUS/WINDOW_GAINED_FOCUS pair
>>>>>>>
>>>>>>> The test should be ready for the infinite loop and I can suggest to
>>>>>>> simply
>>>>>>> stop requesting focus after a couple of iterations.
>>>>>>>
>>>>>>> This could be a compromize. What do you think, Oleg, Mikhail?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Anton.
>>>>>>>
>>>>>>> [1]
>>>>>>>
>>>>>>>
>>>>>>> https://docs.oracle.com/javase/8/docs/api/javax/swing/JComponent.html#requestFocus-- 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Regards, Oleg.
>>>>>>>>
>>>>>>>> P.S. feel free to ask more questions if you need.
>>>>>>>>
>>>>>>>> On Fri, Nov 14, 2014 at 5:07 PM, Anton V. Tarasov
>>>>>>>> <anton.tarasov at oracle.com> wrote:
>>>>>>>>> Hi Mikhail,
>>>>>>>>>
>>>>>>>>> Looks fine for me. Thanks! This was an old one... Do you have any
>>>>>>>>> plans
>>>>>>>>> to
>>>>>>>>> fix it in 8/9 as well?
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Anton.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 14.11.2014 18:57, mikhail cherkasov wrote:
>>>>>>>>>> Hello all,
>>>>>>>>>>
>>>>>>>>>> Could you please review a simple fix of closed test, the test 
>>>>>>>>>> was
>>>>>>>>>> moved
>>>>>>>>>> to
>>>>>>>>>> open repo
>>>>>>>>>> and requestFocus was replaced requestFocusInWindow.
>>>>>>>>>>
>>>>>>>>>> Because "requestFocus" cause infinite war for focus between two
>>>>>>>>>> windows,
>>>>>>>>>> however this
>>>>>>>>>> behavior is correct and doesn't violet spec.
>>>>>>>>>>
>>>>>>>>>> [TESTBUG]
>>>>>>>>>> closed/java/awt/Focus/ActivateFocusTest/ActivateFocusTest.html
>>>>>>>>>> fails intermittently
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8047961
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~mcherkas/8047961/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Mikhail.
>>>>>>>>>>
>