<AWT Dev> [7u4] Review request for 7147435: closed/java/awt/Toolkit/Headless/WrappedToolkitTest/WrappedToolkitTest.sh failed since 7u4b11

[Artem Ananiev]

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 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

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:
>
>> 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

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 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