Allow new dnd api with tree views

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

11 years ago
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.
Assignee

Comment 1

10 years ago
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)
Assignee

Comment 2

10 years ago
Posted file testcase
This test can form a manual test. Just drag the "Drag Me" box onto the tree.
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.)
Assignee

Comment 4

10 years ago
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 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+
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?)
Assignee

Comment 8

10 years ago
(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.
Assignee

Comment 9

10 years ago
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

10 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+
Assignee

Comment 11

10 years ago
http://hg.mozilla.org/mozilla-central/rev/5fca16b4c17f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 500247
Perhaps worth filing a bug to create a way to test DND automatically?
Flags: in-testsuite?
Keywords: dev-doc-needed

Updated

10 years ago
Depends on: 503966

Comment 13

10 years ago
This caused bug 503966
In which version of Gecko will this ship?
Assignee

Comment 15

10 years ago
(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
You need to log in before you can comment on or make changes to this bug.