[7u4] Review request for 7147435: closed/java/awt/Toolkit/Headless/WrappedToolkitTest/WrappedToolkitTest.sh failed since 7u4b11 (original) (raw)

Artem Ananiev artem.ananiev at oracle.com
Tue Feb 28 10:05:50 PST 2012

On 2/28/2012 9:32 AM, Anthony Petrov wrote:

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 awtLoadLibrary.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 AWTTOOLKIT var on X11. A custom toolkit may be specified via the -Dawt.toolkit= argument. The AWTTOOLKIT env var must override this setting. Therefore, the var must

I don't think so. awt.toolkit is a system property referenced in JavaDoc (see Toolkit#getDefaultToolkit() for details), but AWT_TOOLKIT is just an optional hint. The system property must take a precedence over the env variable.

be checked. Please note that the test in question fails on X11 exactly for this reason, it invokes:

_AWTTOOLKIT=XToolkit ${TESTJAVA}/bin/java -Djava.awt.headless=true _ _-Dawt.toolkit=sun.awt.motif.MToolkit _ TestWrapped sun.awt.X11.XToolkit and expects the AWTTOOLKIT 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 awtLoadLibrary.c - I realize it's used by default (inherited from javapropsmd.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

OK, let it be so.

Thanks,

Artem

-- 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 javapropsmd.c:431 is already in the workspace... Other comments: 1. HeadlessGraphicsEnvironment is in the sun.java2d package, not sun.awt 2. Changes to awtLoadLibrary.c look fine as all the defaults are set in javaprops.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/viewbug.do?bugid=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 JNIOnLoad() of libawt.so. Actually, the test WrappedToolkitTest.sh relies on the ability to override the default toolkit by means of setting the AWTTOOLKIT 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

More information about the macosx-port-dev mailing list