455590 - Allow new dnd api with tree views

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Neil Deakin]

nsITreeView has two methods, canDrop and drop, used to specify where items may be dropped. These expect you to check the global drag service api. Instead, we should find a way to use the new api. Perhaps we could just pass the event to these methods as well.

Comment 1

[Neil Deakin]

Attachment: pass the dataTransfer to the treeview

This makes it so we can check the data using the new api, and makes direct use of the dragservice fully obsolete.

Can't make an automated test of this, but a manual test should be easy enough.

Comment 2

[Neil Deakin]

Attachment: testcase

This test can form a manual test. Just drag the "Drag Me" box onto the tree.

Comment 3

[[e-mail]]

Comment on attachment 381738 [details] [diff] [review]
pass the dataTransfer to the treeview

>+  // the dataTransfer field of the event caches the DataTransfer associated
>+  // with the drag. It is initialized when an attempt is made to retrieve it
>+  // rather that when the event is created to avoid duplicating the data when
>+  // no listener ever uses it.

>+// static
>+nsresult
>+nsDOMDragEvent::SetDataTransferInEvent(nsDragEvent* aDragEvent)
This code bears no relation to nsDOMDragEvent, except by way of being one of its callers... if you're going to move it around, why not move it to nsDragEvent itself (or failing that, nsContentUtils)? (That would also make it easier to call from nsTreeBodyFrame.)

>+#include "nsIDOMDataTransfer.h"
You don't actually use the definition, and you already have the declaration.

>+class nsIDOMDataTransfer;
Already declared via nsITreeView.h

>+        NS_ASSERTION(aEvent->eventStructType == NS_DRAG_EVENT, "wrong event type");
>+        nsDragEvent* dragEvent = static_cast<nsDragEvent*>(aEvent);
>+        nsDOMDragEvent::SetDataTransferInEvent(dragEvent);
I guess this counts as a use... seems a shame to create a data transfer just because you drag over a tree that doesn't care. Also, should the return be error-checked? (Maybe not, but you check in the DOMDragEvent code.)

Comment 4

[Neil Deakin]

Attachment: Changed this to use a drag event instead of the data transfer

Comment 5

[[e-mail]]

Comment on attachment 382335 [details] [diff] [review]
Changed this to use a drag event instead of the data transfer

This is going to break people building Thunderbird/SeaMonkey against mozilla-central (rather than mozilla-1.9.1); it might be worth writing a patch to #ifndef the extra arguments for the comm-central callers, or at least giving someone else time to write one before you land this.

>+        nsCOMPtr<nsIPrivateDOMEvent> privevent = do_QueryInterface(domEvent);
>+        privevent->SetTrusted(true);
[I don't quite see the point of this, given that the event never gets dispatched.]

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Creating artificial event for api calls is strange. It would mean that
for the same nsDragEvent there might be two different nsDOMDragEvents.
I think I prefer the first approach. IMO it makes more sense to use
nsIDOMDataTransfer in the API. Don't move the code to nsDragEvent, but to
nsContentUtils.
Would it be possible to "fill" the data transfer object lazyly? So that dragging
over a tree which doesn't care wouldn't cause a real data transfer to be created.
Though, how expensive is creating a data transfer?

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> Though, how expensive is creating a data transfer?

Creating a data transfer while dragging from another app reads numDropItems from an nsIDragSession.  With GTK/X11 (don't know about others), this often requires (and due to bug 497496, always involves, even when not required) fetching information from the other app.

It would be nice to avoid waiting on the other app unnecessarily.
(Though maybe that's something that can/should be sorted out in the nsDOMDataTransfer construction?)

Comment 8

[Neil Deakin]

(In reply to comment #7)
> It would be nice to avoid waiting on the other app unnecessarily.
> (Though maybe that's something that can/should be sorted out in the
> nsDOMDataTransfer construction?)

We could change nsDOMDataTransfer to only cache the external data when one of the methods that uses it is called.

Comment 9

[Neil Deakin]

Attachment: use datatransfer but move code to nsContentUtils

Creating a datatransfer can be expensive for external drags, but it only happens once per drag and then the data is cached already.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 383943 [details] [diff] [review]
use datatransfer but move code to nsContentUtils

>+nsContentUtils::SetDataTransferInEvent(nsDragEvent* aDragEvent)
>+{
>+  if (aDragEvent->dataTransfer)
>+    return NS_OK;
>+
>+  // For draggesture and dragstart events, the data transfer object is
>+  // created before the event fires, so it should already be set. For other
>+  // drag events, get the object from the drag session.
>+  NS_ASSERTION(aDragEvent->message != NS_DRAGDROP_GESTURE &&
>+               aDragEvent->message != NS_DRAGDROP_START,
>+               "draggesture event created without a dataTransfer");

Could you add some check which makes sure that this is called only with
trusted events.

Comment 11

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/5fca16b4c17f

Comment 12

[Nickolay_Ponomarev]

Perhaps worth filing a bug to create a way to test DND automatically?

Comment 13

[Sylvain Pasche]

This caused bug 503966

Comment 14

[Eric Shepherd [:sheppy]]

In which version of Gecko will this ship?

Comment 15

[Neil Deakin]

(In reply to comment #14)
> In which version of Gecko will this ship?

1.9.2

The change is just an extra argument to nsITreeView::canDrop and nsITreeView::drop

Comment 16

[Eric Shepherd [:sheppy]]

This is now documented in:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsITreeView