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

[Anthony Petrov]

On 2/29/2012 12:16 AM, Artem Ananiev wrote:
>>> 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.

Well, the test has been written by you. :)


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

Fine by me. We'll review a new fix with you.

--
best regards,
Anthony


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