<AWT Dev> [9] Review Request: JDK-8030710 [macosx] closed/java/awt/Clipboard/ImageTransferTest/ImageTransferTest times out

Fri Mar 14 13:20:15 UTC 2014

Sorry, I've fixed the wrong test!

We have 2 tests with the same name, one for clipboard and 1 for DnD and I've accidentally fixed the wrong test.

I've created a new bug for this change: https://bugs.openjdk.java.net/browse/JDK-8037371
I'll fix the Clipboard test under 8030710.

With best regards. Petr.

On 14.03.2014, at 16:49, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:

> Hi, Petr.
> The fix looks good to me too.
> 
> On 3/7/14 3:58 PM, Anthony Petrov wrote:
>> This looks fine. Thank you.
>> 
>> -- 
>> best regards,
>> Anthony
>> 
>> On 3/7/2014 4:02 PM, Petr Pchelko wrote:
>>> Hello, Anthony.
>>> 
>>> Please see the updated version here: http://cr.openjdk.java.net/~pchelko/9/8030710/webrev.03/
>>> 
>>> I've added the autorelease to the imageRep. Checked that no memory leaks in the new methods are present.
>>> 
>>> With best regards. Petr.
>>> 
>>> On 07.03.2014, at 15:15, Anthony Petrov <anthony.petrov at oracle.com> wrote:
>>> 
>>>> Hi Petr,
>>>> 
>>>> src/macosx/native/sun/awt/CImage.m:
>>>>> 452     NSBitmapImageRep* imageRep = CImage_CreateImageRep(env, buffer, width, height);
>>>>> 453     if (imageRep) {
>>>>> 454         NSData *tiffImage = [imageRep TIFFRepresentation];
>>>>> 455         jsize tiffSize = (jsize)[tiffImage length];
>>>>> 456         result = (*env)->NewByteArray(env, tiffSize);
>>>>> 457         CHECK_NULL_RETURN(result, nil);
>>>>> 458         jbyte *tiffData = (jbyte *)(*env)->GetPrimitiveArrayCritical(env, result, 0);
>>>>> 459         CHECK_NULL_RETURN(tiffData, nil);
>>>>> 460         [tiffImage getBytes:tiffData];
>>>>> 461         (*env)->ReleasePrimitiveArrayCritical(env, result, tiffData, 0);
>>>>> 462         [imageRep release];
>>>> 
>>>> I see that we're managing the imageRep manually. So it is not a part of the auto-release pool created with the JNF_COCOA_ENTER macro. Which means that the return statements at lines 457 and 459 will lead to a memory leak. I suggest to use @finally to ensure that the imageRep gets released (this is what the JNF_COCOA_EXIT does, too).
>>>> 
>>>> 
>>>> -- 
>>>> best regards,
>>>> Anthony
>>>> 
>>>> On 3/7/2014 10:59 AM, Petr Pchelko wrote:
>>>>> Hello,
>>>>> 
>>>>> Please review the updated version of the fix: http://cr.openjdk.java.net/~pchelko/9/8030710/webrev.02/
>>>>> 
>>>>> I've moved the image handling code from DataTransferer to CImage. This let us reuse the existing code that creates an NSBitmapImageRep from the int array and let us get rid of some JNI upcalls.
>>>>> 
>>>>> Tested with all SQE datatransfer tests and out reg tests. No new failures.
>>>>> 
>>>>> With best regards. Petr.
>>>>> 
>>>>> On 06.03.2014, at 19:38, Petr Pchelko <petr.pchelko at oracle.com> wrote:
>>>>> 
>>>>>> Hello, Sergey.
>>>>>> 
>>>>>> To retrieve the image from native we are using NSDeviceRGBColorSpace (see CImage.m), so we need to use the same color space to put the image into native. If i revert this line no calibrated space the test would fail because the image we get from native is different from the one we put.
>>>>>> 
>>>>>> I’ve just got an idea. CImage API is able to put the Java Image into native, so we could probably reuse it here. Let me investigate this, I’ll write you back when I succeed or fail.
>>>>>> 
>>>>>> With best regards. Petr.
>>>>>> 
>>>>>> 06 марта 2014 г., в 7:22 после полудня, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> написал(а):
>>>>>> 
>>>>>>> Hi, Petr.
>>>>>>> How NSCalibratedRGBColorSpace change affects the fix?
>>>>>>> 
>>>>>>> On 3/6/14 5:30 PM, Petr Pchelko wrote:
>>>>>>>> Hello again.
>>>>>>>> 
>>>>>>>> I’ve updated the fix. The new version is here:
>>>>>>>> http://cr.openjdk.java.net/~pchelko/9/8030710/webrev.01/
>>>>>>>> 
>>>>>>>> 1. I’ve modified the test so it now doesn’t need .html file, also copyright was added.
>>>>>>>> 2. I’ve replaced the public.jpeg mime with a predefined kUTTypeJPEG (which is the same).
>>>>>>>> No such constant in Cocoa, only in Carbon.
>>>>>>>> 
>>>>>>>> With best regards. Petr.
>>>>>>>> 
>>>>>>>> 06 марта 2014 г., в 2:56 после полудня, Petr Pchelko <petr.pchelko at oracle.com> написал(а):
>>>>>>>> 
>>>>>>>>> Hello, Sergey.
>>>>>>>>> 
>>>>>>>>>> I guess JFIF is not a typo in CDataTransferer?
>>>>>>>>> Nop. http://en.wikipedia.org/wiki/JPEG_File_Interchange_Format
>>>>>>>>> Also please see the macosx flavormap.properties type. We call a native as a JFIF and it’s left as is because
>>>>>>>>> it’s called like this on other platforms.
>>>>>>>>> 
>>>>>>>>>> Do we really need ImageTransferTest.html in the test?
>>>>>>>>> The test is for interprocess DnD, so we run the first process as an applet from jtreg.
>>>>>>>>> The applet then starts a second processes main(). I’ll check if I could use 2 main() in one file and if it’s fine for jtreg.
>>>>>>>>> 
>>>>>>>>>> Also copyright header is missing.
>>>>>>>>> Thank you. I’ll update the fix shortly.
>>>>>>>>> 
>>>>>>>>> With best regards. Petr.
>>>>>>>>> 
>>>>>>>>> 06 марта 2014 г., в 2:49 после полудня, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> написал(а):
>>>>>>>>> 
>>>>>>>>>> Hi, Petr.
>>>>>>>>>> I guess JFIF is not a typo in CDataTransferer? Do we really need ImageTransferTest.html in the test?
>>>>>>>>>> Also copyright header is missing.
>>>>>>>>>> 
>>>>>>>>>> On 3/5/14 8:23 PM, Petr Pchelko wrote:
>>>>>>>>>>> Hello, AWT Team.
>>>>>>>>>>> 
>>>>>>>>>>> Please review the fix for the issue:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8030710
>>>>>>>>>>> The fix is available at:
>>>>>>>>>>> http://cr.openjdk.java.net/~pchelko/9/8030710/webrev/
>>>>>>>>>>> 
>>>>>>>>>>> The problem was that on Mac we thought that re support PNG and JPEG image formats, but in reality we did not have mappings for these formats. I've added the missing mappings.
>>>>>>>>>>> 
>>>>>>>>>>> The test is being open sourced. The fix was checked with out tests and with native-Java DnD.
>>>>>>>>>>> 
>>>>>>>>>>> With best regards. Petr.
>>>>>>>>>> 
>>>>>>>>>> -- 
>>>>>>>>>> Best regards, Sergey.
>>>>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> -- 
>>>>>>> Best regards, Sergey.
>>>>>>> 
>>>>>> 
>>>>> 
>>> 
> 
> 
> -- 
> Best regards, Sergey.
> 