Re: [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.tara...@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.tara...@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.tara...@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.tara...@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.