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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 >>>>>> <[email protected]> написал(а): >>>>>> >>>>>>> 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 >>>>>>>> <[email protected]> написал(а): >>>>>>>> >>>>>>>>> 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 >>>>>>>>> <[email protected]> написал(а): >>>>>>>>> >>>>>>>>>> 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. >
