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

[Anton V. Tarasov]

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.
>>>>>>>>>