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

[Sergey Bylokhov]

22.05.2012 12:14, Andrew Brygin wrote:
> 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?
yes, if there is no reason to have different messages.
>
> 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
>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.