Thank you, Sergey, for review and approval of this fix.
Hello reviewers,
Can anybody else become the second reviewer of this fix?
Thank you,
Anton
On 27/08/2020 03:03, Sergey Bylokhov wrote:
Looks fine.
On 24.08.2020 16:26, Anton Litvinov wrote:
Hi Sergey,
Thank you for confirmation about which code in the function
"AwtFrame::WmSize" you suggested to move to Java code level. I agree
with your suggestion and also came to a conclusion in my previous
e-mail that only this code can be moved out. The new version of the
fix, which implements your suggestion, was created and tested. Could
you please look at the second fix version.
Webrev (the 2nd version):
http://cr.openjdk.java.net/~alitvinov/8249183/jdk16/webrev.01
I studied the involved C++ code flow and confirm that the next 3 C++
function calls from the defined by you part of the function
"AwtFrame::WmSize" do not alter any fields of the involved C++ object
"AwtFrame", "AwtDialog" and are just nothing else then the next 3
corresponding Java expressions:
1. C++ function call
"SendWindowEvent(java_awt_event_WindowEvent_WINDOW_ICONIFIED);" -->
Java expression "sun.awt.windows.WComponentPeer.postEvent(new
TimedWindowEvent((Window) target, WindowEvent.WINDOW_ICONIFIED, null,
0, 0, System.currentTimeMillis()));"
2. C++ function call
"SendWindowEvent(java_awt_event_WindowEvent_WINDOW_DEICONIFIED);" -->
Java expression "sun.awt.windows.WComponentPeer.postEvent(new
TimedWindowEvent((Window) target, WindowEvent.WINDOW_DEICONIFIED,
null, 0, 0, System.currentTimeMillis()));"
3. C++ function call "SendWindowStateEvent(oldState, newState);" -->
Java expression "sun.awt.windows.WComponentPeer.postEvent(new
TimedWindowEvent((Window) target, WindowEvent.WINDOW_STATE_CHANGED,
null, oldState, newState, System.currentTimeMillis()));"
Regarding your remark about the need to take into account
"getExtendedState" Java method, I agree with you and confirm that it
is impossible that this method will be called on not instance of
"WFramePeer" Java class and agree that there is no need to change
code related to "getExtendedState" method.
CHANGES IN THE SECOND FIX VERSION:
Change 1 - The pointed by you part of "AwtFrame::WmSize" function was
reimplemented in Java language as the new method
"sun.awt.windows.WWindowPeer.notifyWindowStateChanged(int oldState,
int newState)" in the file "WWindowPeer.java". JNI Invocation of this
new Java method was wrapped in the new C++ function "void
AwtWindow::NotifyWindowStateChanged(jint oldState, jint newState)" in
the file "awt_Window.cpp".
Change 2 - The function "void SendWindowStateEvent(int oldState, int
newState);" was removed from the C++ class "AwtFrame" in the files
"awt_Frame.h", "awt_Frame.cpp", because it is not called from
anywhere in JDK source code after this fix.
Change 3 - Java method ID variable "static jmethodID
setExtendedStateMID;" with code using it was removed from the C++
class "AwtFrame" in the files "awt_Frame.h", "awt_Frame.cpp", because
it is not called from anywhere in JDK source code after this fix.
Thank you,
Anton
On 20/08/2020 05:21, Sergey Bylokhov wrote:
On 19.08.2020 09:16, Anton Litvinov wrote:
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.
No I did not meant to move the whole "AwtFrame::WmSize" to the java,
but only the part related for notification, same as on mac.
So instead of calling " AwtFrame::setExtendedStateMID," from the
native, just call something like "WWindowPeer.notifyState(int old,
int new, zoom, iconify,... etc)", and on
the java side call all necessary things like on mac:
http://hg.openjdk.java.net/jdk/jdk/file/6b984aa424e3/src/java.desktop/macosx/classes/sun/lwawt/LWWindowPeer.java#l661
- postEvent iconify/zoom;
- postWindowStateChangedEvent(newWindowState);
etc
Probably this part of the code in awt_Frame.cpp can be moved to
"WWindowPeer.notifyState(...)?
DTRACE_PRINTLN2("AwtFrame::WmSize: reporting state change %x
-> %x",
oldState, newState);
// sync target with peer
JNIEnv *env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2);
env->CallVoidMethod(GetPeer(env),
AwtFrame::setExtendedStateMID, newState);
// report (de)iconification to old clients
if (changed & java_awt_Frame_ICONIFIED) {
if (newState & java_awt_Frame_ICONIFIED) {
SendWindowEvent(java_awt_event_WindowEvent_WINDOW_ICONIFIED);
} else {
SendWindowEvent(java_awt_event_WindowEvent_WINDOW_DEICONIFIED);
}
}
// New (since 1.4) state change event
SendWindowStateEvent(oldState, newState);
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".
Do we really need to change the code related to the
"getExtendedState" during creation of AWTFrame?
That one should never be used by the AwtDialog, so the crash is kind
of assertion that something go wrong already.
