Review request for 7129732 - [macosx] JCK failure: no focus transfer back to Window owner

[Artem Ananiev]

The fix looks fine.

Unrelated question: should LWWindowPeer.updateFocusableWindowState() be 
called before platformWindow.setVisible()?

Thanks,

Artem

On 1/31/2012 5:12 PM, Anton V. Tarasov wrote:
> Hi Anthony,
>
> On 1/30/12 6:30 PM, Anthony Petrov wrote:
>> Hi Anton,
>>
>> On 01/30/12 16:05, Anton V. Tarasov wrote:
>>> A question to the reviewers. Do we still need to invoke on EDT?
>>> 236 // it is important to call this method on EDT
>>> 237 // to prevent the deadlocks during the painting of the lightweight
>>> delegates
>>> 238 //TODO: WHY? This is a native-system related call. Perhaps NOT
>>> calling
>>> 239 // the painting procedure right from the setVisible(), but rather
>>> relying
>>> 240 // on the native Expose event (or, scheduling the repainting
>>> asynchronously)
>>> 241 // is better?
>>> 242 SwingUtilities.invokeLater(new Runnable() {
>>> 243 @Override
>>> 244 public void run() {
>>> 245 platformWindow.setVisible(visible);
>>
>> This "WHY?..." comment was added by me long time ago. If all the
>> invoked methods may be called on other threads I don't see a reason to
>> dispatch this code to the EDT. However, if you wish to remove this
>> invokeLater(), you'll have to thoroughly test that nothing is broken.
>>
>> We might want to ask Alex (CC'ed) since he added this invokeLater()
>> originally IIRC. Is this still relevant? Doesn't the painting
>> operation gets dispatched to the EDT automatically? May there still be
>> any painting artifacts should the platformWindow.setVisible() be
>> invoked on non-EDT thread?
>
> Oh, I see that I'm better to separate this investigation (due to the
> urgency of the fix). Thanks for your comments!
>
>>
>>
>>
>>>> Please review a fix for 7129732.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ant/7129732/webrev.0/
>>
>> At line 255 a check for !visible is unnecessary since the else branch
>> already ensures that visible is false.
>
> Indeed. I added the first if-statement and forget to refine the second
> one. Thanks for catching this.
>
> Please, see the improved version:
>
> http://cr.openjdk.java.net/~ant/7129732/webrev.1
>
> Thanks,
> Anton.
>
>>
>> Otherwise looks good.
>>
>> --
>> best regards,
>> Anthony
>>
>>
>>>>
>>>> When a focused simple window is getting closed, focus should be
>>>> transferred to the owner.
>>>>
>>>> Thanks,
>>>> Anton.
>>>>
>>>>
>>>
>