Hi Artem,
Thanks for the review. Please find my comments below.
On 2/28/2012 9:22 PM, Artem Ananiev wrote:
http://cr.openjdk.java.net/~anthony/7u4-2-headlessTestFailed-7147435.0/
contains changes to the awt_LoadLibrary.c file only. I have provided a
link to the webrev for 7124511 as a reference only, and put it in the
very end of my message to avoid confusion. Please follow the link in the
beginning of the message to review the fix. Thanks.
Here are some comments from my side:
1. On X11, we don't have any other toolkits than XToolkit, so I don't
see any point in checking the AWT_TOOLKIT var on X11.
A custom toolkit may be specified via the -Dawt.toolkit= argument. The
AWT_TOOLKIT env var must override this setting. Therefore, the var must
be checked. Please note that the test in question fails on X11 exactly
for this reason, it invokes:
AWT_TOOLKIT=XToolkit ${TESTJAVA}/bin/java -Djava.awt.headless=true \
-Dawt.toolkit=sun.awt.motif.MToolkit \
TestWrapped sun.awt.X11.XToolkit
and expects the AWT_TOOLKIT to have precedence over the -Dawt.toolkit
setting. So we must check the environment variable on any *NIX platform.
2. On Mac, CToolkit is not explicitly set in awt_LoadLibrary.c - I
realize it's used by default (inherited from java_props_md.c), but it
would be fine to leave a comment in the code.
The comment at line 112 has been added for exactly this reason. It's
already there.
3. AFAIK, there is no need to check global refs for NULL:
DeleteGlobalRef() is ready to handle null refs.
It may be implemented this way. However, its specification [1] doesn't
mention that NULL is a valid argument. Therefore, I prefer to check for
NULL before calling the function.
[1]
http://download.java.net/jdk8/docs/technotes/guides/jni/spec/functions.html#DeleteGlobalRef
--
best regards,
Anthony
Thanks,
Artem
--
best regards,
Anthony
On 2/28/2012 2:01 AM, Artem Ananiev wrote:
Hi, Anthony,
I have an impression your webrev is prepared against an outdated
version of code. For example, the change in java_props_md.c:431 is
already in the workspace...
Other comments:
1. HeadlessGraphicsEnvironment is in the sun.java2d package, not sun.awt
2. Changes to awt_LoadLibrary.c look fine as all the defaults are set
in java_props.c
3. GraphicsEnvironment.java also looks fine.
Thanks,
Artem
On 2/27/2012 6:30 AM, Anthony Petrov wrote:
Hello,
Please review a fix for
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7147435 at:
http://cr.openjdk.java.net/~anthony/7u4-2-headlessTestFailed-7147435.0/
This bug is a regression of 7124511 fixed for the JDK Mac Port [1].
With
that fix the code setting the awt.toolkit and java.awt.graphicsenv
system properties has been removed from JNI_OnLoad() of libawt.so.
Actually, the test WrappedToolkitTest.sh relies on the ability to
override the default toolkit by means of setting the AWT_TOOLKIT
environment variable, and because of the removal the test has failed.
With the fix for 7147435 I'm restoring the removed parts. In order to
not break the fix for 7124511, I'm setting the system properties
only if
the XToolkit has been requested explicitly.
[1] http://cr.openjdk.java.net/~anthony/x-5-forceHeadless.0/
--
best regards,
Anthony
