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

[Artem Ananiev]

On 2/28/2012 10:56 AM, Anthony Petrov wrote:
> 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.

I don't know why the test was written this way even back in 1.6 time 
frame, when both XToolkit and MToolkit were available.

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

The right behavior with -Dawt.toolkit=sun.awt.motif.MToolkit is to throw 
a ClassNotFoundException exception, because there is not such toolkit in 
JDK7. However, I'm also fine if you completely remove this scenario from 
the test.

Thanks,

Artem

> 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