[8] Review request for 7124310: [macosx] "opposite" seems always null in focus events

[Anthony Petrov]

Thanks. The new fix looks good.

--
best regards,
Anthony

On 11/2/2012 2:20 AM, Leonid Romanov wrote:
> 
> On Nov 1, 2012, at 5:48 PM, Anthony Petrov <anthony.petrov at oracle.com 
> <mailto:anthony.petrov at oracle.com>> wrote:
> 
>> Hi Leonid,
>>
>> src/macosx/classes/sun/lwawt/LWWindowPeer.java
>>> 686         Window oppositeWindow = (opposite == null)? null : 
>>> getTarget();
>>
>> I think this should read "opposite.getTarget()", no?
>>
> 
> Yes. Thanks for catching this. Apparently, our focus subsystem somehow 
> been correcting this mistake, so resulting focus events received by 
> frames in my test were fine.
> 
>> src/macosx/native/sun/awt/AWTWindow.m
>>> 60 static AWTWindow* lastKeyWindow = nil;
>>
>> From previous experience I can tell you that storing a pointer to an 
>> ObjC object w/o retaining it is pointless and may lead to bugs that 
>> are very hard to debug. Please add retain/release calls in the setter 
>> for this variable.
> 
> Done.
> 
> 
>>
>>> 549     AWTWindow *opposite = nil;
>>
>>> 521         jobject oppositeWindow = [opposite.javaPlatformWindow 
>>> jObjectWithEnv:env];
>>
>> At line 549 the opposite may be set to nil, however, at line 512 
>> you're accessing it w/o a check for nil.
>>
> 
> Not necessary. Objective C allows sending messages to nil. The result is 
> nil/FALSE.
> 
> Here is the new webrev:
> http://cr.openjdk.java.net/~leonidr/7124310/webrev.01/
> 
> 
>> --
>> best regards,
>> Anthony
>>
>> On 11/01/12 02:43, Leonid Romanov wrote:
>>> Hi,
>>> Please review a fix for 7124310: [macosx] "opposite" seems always null
>>> in focus events. It's not a real bug, but something that hasn't been
>>> implemented yet: Cocoa focus notifications don't have information about
>>> "opposite" window, so we have to track it ourselves.
>>>
>>> Webrev: http://cr.openjdk.java.net/~leonidr/7124310/webrev.00/
>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7124310
>>>
>>> Thanks,
>>> Leonid.
>