<AWT Dev> Request for review: 7154778 - NSView-based embedded frame

[Anthony Petrov]

On 11/30/2012 2:42 PM, Petr Pchelko wrote:
> Anthony wrote:
>> 4. CWrapper.m
>> You could use the new ThreadingUtilities machinery here as well.
> 
> 	We do not need to do it to fix this bug, so may be it would be better to refactor CWrapper later when we will change everywhere?

But you're adding a new method in this file and do the following:
>  665     [JNFRunLoop performOnMainThreadWaiting:NO withBlock:^(){
>  666         [view setHidden:(BOOL)toHide];
>  667     }];

I don't insist, though. This indeed could wait till we refactor all the 
rest of our code.

The latest fix looks fine to me.

--
best regards,
Anthony


> All the suggestion I did not answer explicitly are used for the changes in the code.
> 
> Best, Petr.
> 
>> On Nov 28, 2012, at 7:09 PM, Anthony Petrov wrote:
> 
>> Hi Petr,
>>
>> 1. src/macosx/native/sun/osxapp/ThreadUtilities.h
>> I suggest to introduce a static (+) method in the ThreadUtilities class rather than a new global macro.
>>
>> Also, while I agree with Sergey that eventually we should use it everywhere in AWT, I suggest to replace other occurrences of this code later, with a separate refactoring fix. Use the new method only for the code that you change/introduce with this fix.
>>
>> BTW, in case wait==NO, we should force going through the JNFRunLoop even if we're running on the main thread.
>>
>>
>> 2. LWWindowPeer.java, CPlatformWindow.java, and peerType
>> It looks like the CPlatformWindow doesn't even need to know the peerType. I think we can safely drop the peerType argument from CPlatformWindow constructor, and only store it in the LWWindowPeer itself. Should we need the value in the future, we can always ask the LWWindowPeer since CPW keeps a reference to the peer object.
>>
>>
>> 3. src/macosx/classes/sun/lwawt/macosx/CMouseInfoPeer.java
>> I think we could avoid using the instancof and instead introduce a method in the PlatformWindow interface (like isUnderMouse()). The CPW and CVPEF would simply provide different implementations for this method.
>>
>>
>> 4. CWrapper.m
>> You could use the new ThreadingUtilities machinery here as well.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 11/27/2012 1:55 PM, Petr Pchelko wrote:
>>> Hello, AWT team.
>>> please, review the following fix for 7154778:
>>> http://cr.openjdk.java.net/~art/pchelko/7154778/
>>> The bug description and evaluation is available here:
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7154778
>>> To work with the new EmbeddedFrame implementation, existing code (e.g. SWT) needs to be modified to use sun.lwawt.macosx.CViewEmbeddedFrame class instead of Apple's apple.awt.CEmbeddedFrame. Here is the corresponding patch for SWT:
>>> http://cr.openjdk.java.net/~art/pchelko/7154778/swt_patch.txt
>>> Best, Petr Pchelko
>