<AWT Dev> [8] Review Request for CR JDK-8020371 [macosx] applets with Drag and Drop fail with IllegalArgumentException

[Anthony Petrov]

Hi Petr,

The fix looks good to me. BTW, could we provide some meaningful message 
with the InvalidDnDOperationException ? E.g. state that it's unsupported 
for this window or something like this? Also, the comments "// 
Unsupported" after the if()return; could be better worded as 
"Unsupported for a window w/o a native view (plugin)".

--
best regards,
Anthony

On 07/11/13 17:50, Petr Pchelko wrote:
> Hello, Artem.
>
> I've updated the fix, the new version is here:
> http://cr.openjdk.java.net/~pchelko/8020371/webrev.01/
>
>> 1. Are CDropTarget and CDragSourceContextPeer ready to handle the case, when their nativePtr is 0?
> 	1. CDropTarget: it's absolutely ready, it has only create and release methods which are ready to have a nativePtr=0.
> 	In native we create a delegate and set it as the DropTarget to the View. This DropTarget get's notified when NSView see some DnD operation.
> 	So we cannot and should not set it for applets.
>
> 	2. CDragSourceContextPeer - it has only one public method: startDrag which could be called when a DragGestureRecognizer recognizes the Drag. In applets it could never happen,
> 	so the CDragSourceContextPeer wouldn't event be created. I'be updated it just to make sure we would never get a native crash with 0 viewPtr.
>
>> 2. Lines 110-113 in CDragSourceContextPeer should be better moved to before line 140.
> 	I've updated the fix.
>
>> 3. It's unclear if CDropTarget and CDragSourceContextPeer should be used for lightweight components at all. How lightweight components are handled on other platforms.?
> Not sure about other platforms, but on Mac it works as follows:
> 	When a DropTarget is set to a lightweight component we set to it's peer and traverse the tree up until we get a non-lightweight peer. And create, update (or don't update) the
> 	CDropTarget for that peer. The CDragSourceContextPeer is a singleton. When DnD is started we update the trigger in it. The trigger could be any component. To get native support
> 	we traverse up the tree from the trigger t get a first heavyweight component and use it as a native peer for DnD.
>
> I\m not sure I've answered your question thought.
> 	
> With best regards. Petr.
>
> On Jul 11, 2013, at 4:15 PM, Artem Ananiev wrote:
>
>> Hi, Petr,
>>
>> I'm not an expert in this code, so a few questions about the fix:
>>
>> 1. Are CDropTarget and CDragSourceContextPeer ready to handle the case, when their nativePtr is 0?
>>
>> 2. Lines 110-113 in CDragSourceContextPeer should be better moved to before line 140.
>>
>> 3. It's unclear if CDropTarget and CDragSourceContextPeer should be used for lightweight components at all. How lightweight components are handled on other platforms.?
>>
>> Thanks,
>>
>> Artem
>>
>> On 7/11/2013 1:51 PM, Petr Pchelko wrote:
>>> Hello, AWT Team.
>>>
>>> Please review the fix for the issue:
>>> http://bugs.sun.com/view_bug.do?bug_id=8020371
>>> (It's recently created, so it might not be visible on bugs.sum.com yet)
>>> The fix is available at:
>>> http://cr.openjdk.java.net/~pchelko/8020371/webrev/
>>>
>>> The problem:
>>> We currently do not support DnD in applets on Mac, so IllegalArgumentException was thrown. If you explicitly call component.setDropTarget(new DropTarget()) the applet would fail to start due to this exception.
>>>
>>> The solution:
>>> The fix removes the exception and silently ignores unsupported DnD initialization.
>>> DnD does not start to work, however at least an applet could be started.
>>> However, I'm not sure if we should print a warning about unsupported DnD. What do you think?
>>>
>>> With best regards. Petr.
>>>
>