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

[steve.x.northover at oracle.com]

I see lots of changes to the SWT side and will need something I can run 
to really evaluate them.  I'm ok with waiting until the JDK side of the 
code is in a public download rather than building the JDK myself.  The 
JDK changes need to be released before the Eclipse side in any case.

Steve

On 06/12/2012 9:32 AM, Sergey Bylokhov wrote:
> Hi, Petr.
> jdk part looks fine to me.
> Just one hint it would be good to add some documentation to 
> CViewEmbeddedFrame is a kind of public api and this class should not 
> be changed/moved/renamed etc. It is not necessary to send additional 
> webrev for this.
>
> 06.12.2012 17:42, Petr Pchelko wrote:
>> 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
>
>