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

[Sergey Bylokhov]

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.