Allow new dnd api with tree views

RESOLVED FIXED

Status

()

Core
Drag and Drop
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Neil Deakin (not available until Aug 9), Assigned: Neil Deakin (not available until Aug 9))

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Created attachment 381738 [details] [diff] [review]
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.
Attachment #381738 - Flags: superreview?(Olli.Pettay)
Attachment #381738 - Flags: review?(neil)
Created attachment 381740 [details]
testcase

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

Comment 3

8 years ago
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.)
Created attachment 382335 [details] [diff] [review]
Changed this to use a drag event instead of the data transfer
Attachment #381738 - Attachment is obsolete: true
Attachment #382335 - Flags: superreview?(Olli.Pettay)
Attachment #382335 - Flags: review?(neil)
Attachment #381738 - Flags: superreview?(Olli.Pettay)
Attachment #381738 - Flags: review?(neil)

Comment 5

8 years ago
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.]
Attachment #382335 - Flags: review?(neil) → review+

Comment 6

8 years ago
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?
(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?)
(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.
Created attachment 383943 [details] [diff] [review]
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.
Attachment #382335 - Attachment is obsolete: true
Attachment #383943 - Flags: superreview?(Olli.Pettay)
Attachment #383943 - Flags: review?(neil)
Attachment #382335 - Flags: superreview?(Olli.Pettay)

Updated

8 years ago
Attachment #383943 - Flags: review?(neil) → review+
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.
Attachment #383943 - Flags: superreview?(Olli.Pettay) → superreview+
http://hg.mozilla.org/mozilla-central/rev/5fca16b4c17f
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 500247

Comment 12

8 years ago
Perhaps worth filing a bug to create a way to test DND automatically?
Flags: in-testsuite?
Keywords: dev-doc-needed

Updated

8 years ago
Depends on: 503966

Comment 13

8 years ago
This caused bug 503966
In which version of Gecko will this ship?
(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
This is now documented in:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsITreeView
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.