[8] request for review: 7146550: [macosx] DnD test failure in createCompatibleWritableRaster()

[Andrew Brygin]

Hi Sergey,

  please see my comments inline.
On 21.05.2012 16:28, Sergey Bylokhov wrote:
> Hi. Andrew.
> Fix looks good.
> But I have another 2 question.
> 1 Is it possible to leak Graphics object in the CImage.imageToArray()? 
> probably g2.dispose(); should be called in the finally block?
I do not think so: the toolkit image which we are going to draw is 
already prepared and has a good state,
so I do not see why do we need a finally block here. Could you please 
explain in more details what scenario
do you have in mind?

It is not a bug deal to add a finally block here, but we have be able to 
prevent a leak even if dispose() method
was not invoked, because the spec does not make such claims. If we 
strongly need it to be called, I think we
are in trouble...

> 2 In the windows implementation  ex.getMessage() added to "drag image 
> creation problem:". Also exception text a bit different(dot and colon).
It is a statement. What is the question? Would you like to see the 
exception message to be exactly same as on windows?

Looking for two next questions :)

Thanks,
Andrew
>
>
> 21.05.2012 16:07, Andrew Brygin wrote:
>> Hi Sergey,
>>
>>  thanks for the review!
>>
>>  I have updated the fix according to your comments:
>>  - debug output is removed,
>>  - image with zero dimension is accepted.
>>
>>  Please take a look to updated webrev:
>>  http://cr.openjdk.java.net/~bae/7146550/webrev.01/
>>
>> Thanks,
>> Andrew
>>
>> On 21.05.2012 15:55, Sergey Bylokhov wrote:
>>> Hi, Andrew.
>>> Two questions:
>>> 1 Probably System.err.println("createFromImage: buffer: " + 
>>> buffer);  should be removed from CImage.createFromImage()?
>>> 2 I guess windows implementation uses images with width&height=0 as 
>>> the correct image. Should we do the same?
>>>
>>> 17.05.2012 16:34, Andrew Brygin wrote:
>>>> Hello,
>>>>
>>>>  could you please review a fix for 7146550?
>>>>
>>>>  DnD code does not check whether a toolkit image is ready to be 
>>>> painted
>>>>  into a buffer. We have to do this check in order to get correct image
>>>>  dimension.
>>>>
>>>>  Suggested fix just delegates the conversion of toolkit image into a
>>>>  CImage instance to a helper class CImage.Creator. This helper class
>>>>  provides two ways to convert a toolkit image int CImage:
>>>>
>>>>    - immediate conversion: returns valid CImage only if toolkit image
>>>>       has valid dimension, and null otherwise.
>>>>      Immediate conversion works fine for toolkit images which are
>>>>      prepared in advance, or are based on memory image source.
>>>>
>>>>    - normal conversion: a toolkit image is prepared for rendering 
>>>> using
>>>>       a media tracker.
>>>>
>>>>  We can not use the normal conversion in case of DnD, because broken
>>>>  toolkit image (whose producer never updates the image state for any
>>>>  reasons) can cause a hang in the DnD the code (test in question
>>>>  implements such scenario).
>>>>
>>>>  If supplied drag image can not be conversed to CImage (either an
>>>>  exception is thrown or it is not ready, i.e. a result of conversion
>>>>  is null for any reasons) the InvalidDnDOperationException is thrown
>>>>  in order to notify user about the problem with drag image.
>>>>  Note that such approach is already used on windows platform, so with
>>>>  this fix, macosx behaves in a uniform way with windows.
>>>>
>>>>  Beside this, this fix pushes the drag image (an instance of CImage)
>>>>  down to native DnD machinery in order to be able to display drag
>>>>  images.
>>>>
>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7146550
>>>> Webrev: http://cr.openjdk.java.net/~bae/7146550/webrev.00/
>>>>
>>>> Thanks,
>>>> Andrew
>>>
>>>
>>
>
>