<AWT Dev> [8] Review Request: CR 8015455 java/awt/dnd/DisposeFrameOnDragCrash/DisposeFrameOnDragTest.java hangs on windows

[Petr Pchelko]

Hello, AWT Team.

This is a reminder. Could I please get a second review on this fix.

With best regards. Petr.

On Aug 21, 2013, at 6:37 PM, Petr Pchelko wrote:

> Hello, Anthony.
> 
> I've updated the fix according to your comments:
> http://cr.openjdk.java.net/~pchelko/8015455/webrev.02/
> 
>> Also, I suggest to run some DnD regression tests to ensure the fix is safe.
> I've run all dnd regression tests on Windows and Mac. No new failures after the fix.
> 
> With best regards. Petr.
> 
> On Aug 21, 2013, at 4:44 PM, Anthony Petrov wrote:
> 
>> Hi Petr,
>> 
>> The new fix looks much better. Thanks!
>> 
>> I'm only concerned with the synchronization scheme. Since the DnD handlers are synchronized methods, the new boolean flag should only be accessed under a synchronized(this){} block in removeNotify() for consistency and thread safety.
>> 
>> Are you sure the cleanup() method needs to be made package-private? I believe it should be accessible from dragExit() even if it stays private because the methods belong to the same outer class.
>> 
>> Also, I suggest to run some DnD regression tests to ensure the fix is safe.
>> 
>> --
>> best regards,
>> Anthony
>> 
>> On 08/21/2013 04:11 PM, Petr Pchelko wrote:
>>> Hello Anthony, Alexandr.
>>> 
>>> Thank you for the reviews.
>>> 
>>> Please hava a look on the new version of this fix:
>>> http://cr.openjdk.java.net/~pchelko/8015455/webrev.01/
>>> 
>>> I have changed the approach according to your comments. DropTarget is tracking if it is under the mouse and sends a synthetic dragExit on removeNotify if necessary.
>>> Also I've added a check in TransferHandler.SwingDropTarget.dragExit to make sure that the cleanup would be called even if the DropTarget is inactive and dragExit events are not dispatched to the dtListener.
>>> 
>>> With best regards. Petr.
>>> 
>>> On Aug 21, 2013, at 1:40 PM, Anthony Petrov wrote:
>>> 
>>>> Hi Petr,
>>>> 
>>>> The java.awt.dnd.DropTarget is a public class, and as such adding any non-private member to it is an API change. I'm not sure we want to expose the dtListener field as a public (well, protected) API because it's too low-level IMO.
>>>> 
>>>> Besides, it seems that in the case if the frame is disposed, an application won't ever receive an event indicating that the DnD operation has ended (IIUC, if it did, the normal processing of the dragExit would reset the timer).
>>>> 
>>>> So, how about overriding the removeNotify() in DropTarget() and sending an appropriate DnD gesture termination event from there?
>>>> 
>>>> 
>>>> --
>>>> best regards,
>>>> Anthony
>>>> 
>>>> On 08/21/2013 01:21 PM, Petr Pchelko wrote:
>>>>> Hello, AWT Team.
>>>>> 
>>>>> Please review the fix for the issue:
>>>>> http://bugs.sun.com/view_bug.do?bug_id=8015455
>>>>> The fix is available at:
>>>>> http://cr.openjdk.java.net/~pchelko/8015455/webrev.00/
>>>>> 
>>>>> The SwingDropTarget initializes a SwingTimer to handle autoscroll. This timer was stopped on dragExit and drop. However, if the frame was disposed in some dropTarget listener the timer was never stopped and prevented the AWT application from exiting.
>>>>> 
>>>>> With best regards. Petr,
>>>>> 
>>> 
>