Hi Sergey,
Thank you very much for review of this fix. I have been evaluating your
suggestion and my opinion is following. On macOS that code was
implemented already from the moment JDK 7 code for macOS appeared.
Although macOS-specific platform-dependent code on Java level has some
similarities with Windows platform-dependent code on Java level, native
platform-dependent code is very different between these two OSes.
Of course it is technically possible to move some part of
"AwtFrame::WmSize" function
(http://hg.openjdk.java.net/jdk/client/file/2c7b5ae39c19/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#l915)
to some new method or methods in "sun.awt.windows.WWindowPeer", but that
fix will be much bigger, will require different changes both in C++ and
Java code, and will be necessary to prove that such refactoring did not
break anything previously working well. "AwtFrame::WmSize" function
depends on:
- "AwtFrame" class fields: "m_iconic", "m_zoomed" through the functions:
"isIconic()", "isZoomed()", "setIconic(BOOL)", "setZoomed(BOOL)".
- Win32 API constants: "SIZE_MINIMIZED", "SIZE_MAXIMIZED".
- Call to the function "AwtWindow::UpdateSecurityWarningVisibility" is
not movable to Java level.
It looks that only contents of "if (changed != 0) {" block
(http://hg.openjdk.java.net/jdk/client/file/2c7b5ae39c19/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#l975)
can be moved to some new Java method. But what to do with JNI invocation
of "getExtendedState()" method
(http://hg.openjdk.java.net/jdk/client/file/2c7b5ae39c19/src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp#l281).
To my mind, such refactoring will not give much benefit, new Java method
or methods will not be used from anywhere else, but it will be required
to change code which currently works stably and maybe the changes will
bring some new problems. I would prefer to make the fix as narrow as
possible for this not very usual scenario leading to the crash.
ALTERNATIVE SUGGESTION FROM ME:
By doing this minimal version of the fix, I tried to avoid even JNI
usage without the need. If you do not like this custom flag, then would
you agree for its change to a normal check of peer object class using
"instanceof" operator?
For example, to use instead of this flag the next if condition in
"awt_Frame.cpp":
"jobject peer = GetPeer();
if ((peer != NULL) && (JNU_IsInstanceOfByName(env, peer,
"sun/awt/windows/WFramePeer") > 0)) {"
Or if you do not like variant with "instanceof" operator, then instead
of introduction any checks anywhere, I can suggest you as another
alternative option to implement 2 new empty private methods: "int
getExtendedState()", "void setExtendedState(int)" in the class
"sun.awt.windows.WWindowPeer".
Thank you,
Anton
On 17/08/2020 21:15, Sergey Bylokhov wrote:
Hi, Anton.
On 14.08.2020 10:00, Anton Litvinov wrote:
Could you please review the following fix for the bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8249183
Webrev: http://cr.openjdk.java.net/~alitvinov/8249183/jdk16/webrev.00
Maybe we can get rid most part of this native method and do all stuff
in java in WWindowPeer?
Something like already done on macOS:
notifyIconify():
http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l661
notifyZoom():
http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l686
So we will be able to use plain "instanceof Frame" instead of custom
flags:
http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l1075
