Re: AWT Dev [8] Review request for 7024749: JDK7 b131---a crash in: Java_sun_awt_windows_ThemeReader_isGetThemeTransitionDurationDefined+0x75
Hi, Oleg, I don't see anything wrong with the fix (which hopefully means that it's fine :)) One cosmetic issue (no need to send a new version for review): could you change ImmGetContext() to ::ImmGetContext(), so it's easy to distinguish between AwtComponent's methods and Win32 calls, please? Thanks, Artem On 6/22/2012 8:28 PM, Oleg Pekhovskiy wrote: Hi, Please review the second version of fix for CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7024749 Webrev: http://cr.openjdk.java.net/~bagiras/8/7024749.2 It resolves applet's IME typing problems that existed with the first fix. Here are my answers to Artem's comments: 1. I see no reason to change ImmGetContext() to ImmGetHWnd(). Everywhere in the code ImmGetHWnd() is followed by ImmGetContext(), and these two calls can easily be combined into a single method in AwtComponent. ImmGetHWnd() ImmGetContext() return HWND and HIMC accordingly that are used BOTH in ImmReleaseContext(). 2. Comment about focus proxy in AwtComponent::OpenCandidateWindow() is now obsolete. Instead, you need to add a comment why we send WM_IME_NOTIFY to this component (GetHWnd()), not to its focus proxy. It's not obsolete now, because I returned GetProxyFocusOwner() there. 3. There is no need to call ImmReleaseContext() if hIMC is NULL in AwtComponent::WmImeSetContext(). Thanks, I changed that. 4. In WM_ACTIVATE handler, I would first handle WM_IME_ENDCOMPOSITION, then call ImmReleaseContext(). Seems like it would be better to release IMM Context for avoiding its simultaneous usage. 5. In the same WM_ACTIVATE handler, what's the reason of direct call to DefWindowProc() instead of regular ::SendMessage()? It's not my code, so I left that as is. PS: this fix leads to the regression for 'test/java/awt/Focus/AppletInitialFocusTest', but I was not able to fix that yet (ready to file a separate CR). Thanks, Oleg
AWT Dev hg: jdk8/awt/jdk: 7142091: [macosx] RFE: Refactoring of peer initialization/disposing
Changeset: 7d1eae258183 Author:serb Date: 2012-06-26 13:46 +0400 URL: http://hg.openjdk.java.net/jdk8/awt/jdk/rev/7d1eae258183 7142091: [macosx] RFE: Refactoring of peer initialization/disposing Reviewed-by: anthony, art ! src/macosx/classes/sun/lwawt/LWButtonPeer.java ! src/macosx/classes/sun/lwawt/LWCheckboxPeer.java ! src/macosx/classes/sun/lwawt/LWChoicePeer.java ! src/macosx/classes/sun/lwawt/LWComponentPeer.java ! src/macosx/classes/sun/lwawt/LWLabelPeer.java ! src/macosx/classes/sun/lwawt/LWListPeer.java ! src/macosx/classes/sun/lwawt/LWScrollBarPeer.java ! src/macosx/classes/sun/lwawt/LWScrollPanePeer.java ! src/macosx/classes/sun/lwawt/LWTextAreaPeer.java ! src/macosx/classes/sun/lwawt/LWTextComponentPeer.java ! src/macosx/classes/sun/lwawt/LWTextFieldPeer.java ! src/macosx/classes/sun/lwawt/LWWindowPeer.java ! src/macosx/classes/sun/lwawt/macosx/CPlatformWindow.java ! src/macosx/native/sun/awt/AWTWindow.m
AWT Dev [7u6] Review request for 7142091: [macosx] RFE: Refactoring of peer initialization/disposing
Hi Everyone, Please review the fix. Notes from the bug and comments: 1. setVisible() should be called at the end of the peers initialization. We can move super.initialize() to the end of the peers initializations. Initialize() was split to initialize() and initializeImpl(). In the initialize() we call initializeImpl and then we call to setVisible(). initializeImpl overridden in subclasses. 2. Invokelater in the initialization/disposing is a tricky. Removed. 3. replaceSurfacedata() should be moved outside of LWWindowPeer.setVisible() Done. Also duplicate code was extracted to setVisible() method which call setVisibleImpl(). 4. Backbuffer in replaceSurfacedata() should be initialized by clearRect instead of fillrect(composite is important). Done. related to composite. 5. During lwwindowpeer initialization we call two similar methods nativeSetNSWindowAlpha() and setAlphaValue(). nativeSetNSWindowAlpha() removed from CPlatformWindow.java. Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7142091 Webrev can be found at: http://cr.openjdk.java.net/~serb/7142091/webrev.01/ Also note that CR 7177173 depends from this one. -- Best regards, Sergey.
Re: AWT Dev [7u6] Review request for 7142091: [macosx] RFE: Refactoring of peer initialization/disposing
Looks good to me. -- best regards, Anthony On 6/26/2012 3:49 PM, Sergey Bylokhov wrote: Hi Everyone, Please review the fix. Notes from the bug and comments: 1. setVisible() should be called at the end of the peers initialization. We can move super.initialize() to the end of the peers initializations. Initialize() was split to initialize() and initializeImpl(). In the initialize() we call initializeImpl and then we call to setVisible(). initializeImpl overridden in subclasses. 2. Invokelater in the initialization/disposing is a tricky. Removed. 3. replaceSurfacedata() should be moved outside of LWWindowPeer.setVisible() Done. Also duplicate code was extracted to setVisible() method which call setVisibleImpl(). 4. Backbuffer in replaceSurfacedata() should be initialized by clearRect instead of fillrect(composite is important). Done. related to composite. 5. During lwwindowpeer initialization we call two similar methods nativeSetNSWindowAlpha() and setAlphaValue(). nativeSetNSWindowAlpha() removed from CPlatformWindow.java. Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7142091 Webrev can be found at: http://cr.openjdk.java.net/~serb/7142091/webrev.01/ Also note that CR 7177173 depends from this one. -- Best regards, Sergey.
Re: AWT Dev [8] Review request for 7024749: JDK7 b131---a crash in: Java_sun_awt_windows_ThemeReader_isGetThemeTransitionDurationDefined+0x75
Hi Oleg, I'm Ok with the kbd focus changes. Thanks, Anton. On 22.06.2012 20:28, Oleg Pekhovskiy wrote: Hi, Please review the second version of fix for CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7024749 Webrev: http://cr.openjdk.java.net/~bagiras/8/7024749.2 It resolves applet's IME typing problems that existed with the first fix. Here are my answers to Artem's comments: 1. I see no reason to change ImmGetContext() to ImmGetHWnd(). Everywhere in the code ImmGetHWnd() is followed by ImmGetContext(), and these two calls can easily be combined into a single method in AwtComponent. ImmGetHWnd() ImmGetContext() return HWND and HIMC accordingly that are used BOTH in ImmReleaseContext(). 2. Comment about focus proxy in AwtComponent::OpenCandidateWindow() is now obsolete. Instead, you need to add a comment why we send WM_IME_NOTIFY to this component (GetHWnd()), not to its focus proxy. It's not obsolete now, because I returned GetProxyFocusOwner() there. 3. There is no need to call ImmReleaseContext() if hIMC is NULL in AwtComponent::WmImeSetContext(). Thanks, I changed that. 4. In WM_ACTIVATE handler, I would first handle WM_IME_ENDCOMPOSITION, then call ImmReleaseContext(). Seems like it would be better to release IMM Context for avoiding its simultaneous usage. 5. In the same WM_ACTIVATE handler, what's the reason of direct call to DefWindowProc() instead of regular ::SendMessage()? It's not my code, so I left that as is. PS: this fix leads to the regression for 'test/java/awt/Focus/AppletInitialFocusTest', but I was not able to fix that yet (ready to file a separate CR). Thanks, Oleg
AWT Dev hg: jdk8/awt/jdk: 7024749: JDK7 b131---a crash in: Java_sun_awt_windows_ThemeReader_isGetThemeTransitionDurationDefined+0x75
Changeset: c66b34ec39c3 Author:bagiras Date: 2012-06-26 16:46 +0400 URL: http://hg.openjdk.java.net/jdk8/awt/jdk/rev/c66b34ec39c3 7024749: JDK7 b131---a crash in: Java_sun_awt_windows_ThemeReader_isGetThemeTransitionDurationDefined+0x75 Reviewed-by: art, ant ! src/windows/native/sun/windows/awt_Component.cpp ! src/windows/native/sun/windows/awt_Component.h ! src/windows/native/sun/windows/awt_FileDialog.cpp ! src/windows/native/sun/windows/awt_Frame.cpp ! src/windows/native/sun/windows/awt_TextComponent.cpp + test/java/awt/Frame/7024749/bug7024749.java
Re: AWT Dev [7u6] Review request for 7142091: [macosx] RFE: Refactoring of peer initialization/disposing
Still looks fine. Thanks, Artem On 6/26/2012 3:49 PM, Sergey Bylokhov wrote: Hi Everyone, Please review the fix. Notes from the bug and comments: 1. setVisible() should be called at the end of the peers initialization. We can move super.initialize() to the end of the peers initializations. Initialize() was split to initialize() and initializeImpl(). In the initialize() we call initializeImpl and then we call to setVisible(). initializeImpl overridden in subclasses. 2. Invokelater in the initialization/disposing is a tricky. Removed. 3. replaceSurfacedata() should be moved outside of LWWindowPeer.setVisible() Done. Also duplicate code was extracted to setVisible() method which call setVisibleImpl(). 4. Backbuffer in replaceSurfacedata() should be initialized by clearRect instead of fillrect(composite is important). Done. related to composite. 5. During lwwindowpeer initialization we call two similar methods nativeSetNSWindowAlpha() and setAlphaValue(). nativeSetNSWindowAlpha() removed from CPlatformWindow.java. Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7142091 Webrev can be found at: http://cr.openjdk.java.net/~serb/7142091/webrev.01/ Also note that CR 7177173 depends from this one. -- Best regards, Sergey.
Re: AWT Dev [7u6] Review request for 7024749: JDK7 b131---a crash in: Java_sun_awt_windows_ThemeReader_isGetThemeTransitionDurationDefined+0x75
Since it's a direct backport, it looks fine. Thanks, Artem On 6/26/2012 5:14 PM, Oleg Pekhovskiy wrote: Hi, Please review this backport for CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7024749 Webrev: http://cr.openjdk.java.net/~bagiras/7u6/7024749.1 Thanks, Oleg
AWT Dev hg: jdk8/awt/jdk: 7124326: [macosx] An issue similar to autoshutdown one in two AppContexts situation.
Changeset: 6d37b95f0555 Author:anthony Date: 2012-06-26 17:29 +0400 URL: http://hg.openjdk.java.net/jdk8/awt/jdk/rev/6d37b95f0555 7124326: [macosx] An issue similar to autoshutdown one in two AppContexts situation. Summary: Don't add SystemTrayPeer to the peers map Reviewed-by: art, leonidr ! src/macosx/classes/sun/lwawt/macosx/LWCToolkit.java
Re: AWT Dev [7u6] Review request for 7024749: JDK7 b131---a crash in: Java_sun_awt_windows_ThemeReader_isGetThemeTransitionDurationDefined+0x75
Thank you! Oleg 6/26/2012 5:27 PM, Anton V. Tarasov wrote: Hi Oleg, I'm ok with the focus related changes. Thanks, Anton. On 26.06.2012 17:14, Oleg Pekhovskiy wrote: Hi, Please review this backport for CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7024749 Webrev: http://cr.openjdk.java.net/~bagiras/7u6/7024749.1 Thanks, Oleg
Re: AWT Dev [7u6] Review request for 7024749: JDK7 b131---a crash in: Java_sun_awt_windows_ThemeReader_isGetThemeTransitionDurationDefined+0x75
Thank you! Oleg 6/26/2012 5:27 PM, Artem Ananiev wrote: Since it's a direct backport, it looks fine. Thanks, Artem On 6/26/2012 5:14 PM, Oleg Pekhovskiy wrote: Hi, Please review this backport for CR: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7024749 Webrev: http://cr.openjdk.java.net/~bagiras/7u6/7024749.1 Thanks, Oleg
