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

[Anthony Petrov]

On 2/28/2012 10:05 PM, Artem Ananiev wrote:
>>> 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.

I agree that the AWT_TOOLKIT isn't documented properly. However, the 
test has been written some time ago and relied on this behavior. And it 
worked just fine before merging the Mac Port changes. So technically 
this is a regression.

Yes, we can just drop this behavior, in which case the fix that you're 
currently reviewing doesn't make sense and should actually change the 
test by eliminating the command line causing this test to fail.

Please clarify if you want this issue to be addressed this way.

--
best regards,
Anthony


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