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

[Petr Pchelko]

Hello, AWT Team.

The updated version of the fix is here: http://cr.openjdk.java.net/~art/pchelko/7154778/webrev-jdk8/
And the SWT patch is here: http://cr.openjdk.java.net/~art/pchelko/7154778/swt_patch.txt

The changes from the previous version of the fix: 
	changed the isViewUnderMouse function in AWTView to use hitTest
	made a view to not resize it's subviews by default. 
All the rest in the fix is the same as in the previous version.

The SWT patch is also slightly changed, now the shell listeners are removed when the parent component is resized.

Best, Petr. 

On Nov 30, 2012, at 2:42 PM, Petr Pchelko wrote:

> Thank you for your feedback, here is the new version of the fix: http://cr.openjdk.java.net/~art/pchelko/7154778/
> And the patch for the SWT is available at: http://cr.openjdk.java.net/~art/pchelko/7154778/swt_patch.txt
> 
> Sergey wrote: 
>> 4 Probably we shall replace performOnMainThread  on AWT_PERFORM_ON_MAIN_THREAD_WAITING  everywhere?
> 	Yes, but it would require too many change unrelated to the bug itself, so may be we should do it later?
> 
>> 5 Where CPlatformView.setAutoResizable is used?
> 	In the initializer of the CViewPlatformEmbeddedFrame.
> 
>> 6 CViewPlatformEmbeddedFrame could cache the native bounds, like CPWindows does? in this way CPlatformView.nativeGetLocationOnScreen is unnecessary.
> 	As I see this is impossible, because CViewEmbeddedFrame knows it's bounds only relative to some hosting superview, not relative to the screen. And as we have now access to the superview in the Java code we need this native method.
> 
>> 7 Why we need override resizeWithOldSuperviewSize() in AWTView? How it work in the usual way when we use AWTWindow windowDidResize/windowDidMove?
> 	NSView does not provide the viewDidResize/viewDidMove notifications. It has only notifications about the liveResize, but it is insufficient because we need to notify Java not only about live resize but about any resize triggered by the superview.
> 
> Leonid wrote:
>> I don't quite understand some focus related changes you've made. What is the reason for generating focus events from - (BOOL) becomeFirstResponder and - (BOOL) resignFirstResponder? There are - (void) windowDidBecomeKey and - (void) windowDidResignKey methods in AWTWindow we've been using to generate focus events from, so I'm afraid that with your change will be getting duplicate focus events.  
> 	At first I wanted to make the EmbeddedFrame manage it's focus itself in order to make as low amount of changes in SWT as possible. But, I think you are right  and this might be dangerous, so I have changed the focus management system. 
> 	Now the hosting application must handle focus management for us by synthesizeWindowActivation/synthesizeWindowDeactivation methods.
> 
> 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?
> 
> 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
>