[e10s] Support link, shortcut, content, and file drag and drop

RESOLVED FIXED in Firefox 40

Status

()

defect
P3
normal
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: TimAbraldes, Assigned: smaug)

Tracking

(Depends on 1 bug)

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(e10sm4+, firefox40 fixed)

Details

(Whiteboard: [browser tabs drag and drop support is bug 1012784])

Attachments

(4 attachments, 15 obsolete attachments)

v20
85.41 KB, patch
enndeakin
: review+
karlt
: review-
karlt
: feedback+
Details | Diff | Splinter Review
v21
87.45 KB, patch
karlt
: review+
Details | Diff | Splinter Review
v22
87.50 KB, patch
Details | Diff | Splinter Review
v25
89.98 KB, patch
Details | Diff | Splinter Review
I am unable to initiate a drag from within an e10s-enabled browser. Also, dragging items into an e10s-enabled browser appears to do nothing.
To be more clear, this includes attempting to drag links, text, images, and tabs from or to an e10s-enabled browser
In Linux, attempting to use drag and drop just crashes. We should at least fix that.
Assignee: nobody → evilpies
Posted patch test (obsolete) — Splinter Review
Posted patch wip (obsolete) — Splinter Review
With this patch we can drop various text based formats from the child into the parent and other applications. The reverse doesn't work yet. For that to work we would need to forward the drag & drop events from the parent to the child.

Right now the child and parent have two independent "drag sessions". I think with some cleverness like sending all the events from parent to the child etc. that might actually be the easiest approach.

When you drag images there is often a small preview next to the mouse cursor, which is also missing. We would render this preview in the child and send it to the parent.
Attachment #8391465 - Attachment is obsolete: true
Duplicate of this bug: 987996
This bug is too big for me to tackle at the moment, pick it up if you want.
Assignee: evilpies → nobody
Assignee: nobody → wmccloskey
Priority: -- → P3
smaug volunteered to investigate drag-and-drop on Linux.
Assignee: wmccloskey → bugs
Move old M2 P3 bugs to M4 (because tracking-e10s=m5+ flag doesn't exist yet).
Duplicate of this bug: 1067730
Loic reported duplicate bug 1067730, but I wanted to share his nice test cases in this bug:

STR: 
Open this DnD demo: http://html5demos.com/drag
Same with DnD and XHR upload: http://html5demos.com/dnd-upload

Result: nothing is DnDed.
Drag and drop will be hard to get working. The two options are:

One key issue is that the data placed in the data transfer will be need to be sent from the child to the parent process. The patch above handles text, but doesn't handle any other kind of data. Maybe that's less important for web pages that only support text and files, but the parent will need to handle non-text data.

I'm not I like the approach used by the patch above though.
We have IPC support for blobs/DOMFiles, so transfering data shouldn't be too difficult.

(I'm a bit late with this because the review queue keeps staying long.)
This bug is not related to dragging browser tabs. For those problems, see bug 1012784 or bug 1068167.
Summary: [e10s] Support drag and drop → [e10s] Support drag and drop of files or HTML content. (This bug is not related to dragging browser tabs.)
OS: Windows 8.1 → All
Hardware: x86_64 → All
On Windows, anything dragged into or out of the main browser window fails. This includes shortcuts dropped into the main browser window to navigate, mozilla urls dragged from the nav bar to the bookmarks bar or out on the desktop.

Testing on osx things look about the same.
Summary: [e10s] Support drag and drop of files or HTML content. (This bug is not related to dragging browser tabs.) → [e10s] Support link, shortcut, and file drag and drop.
Whiteboard: [browser tabs drag and drop support is bug 1012784]
Olli, are you currently working on this?
Flags: needinfo?(bugs)
Blocks: 1067129
No longer blocks: core-e10s, old-e10s-m2
Summary: [e10s] Support link, shortcut, and file drag and drop. → [e10s] Support link, shortcut, content, and file drag and drop
Yes. Was my earlier comment not clear enough? :)
Flags: needinfo?(bugs)
Blocks: 863512
(I don't see why this would block clipboard)
No longer blocks: 1071562
Duplicate of this bug: 1074143
Duplicate of this bug: 1075683
Duplicate of this bug: 1079580
(In reply to Jim Mathies [:jimm] from comment #20)
> On Windows, anything dragged into or out of the main browser window fails.
> This includes shortcuts dropped into the main browser window to navigate,
> mozilla urls dragged from the nav bar to the bookmarks bar or out on the
> desktop.
> 
> Testing on osx things look about the same.

Note we fixed the bookmark / nav bar bugs so chrome is working now.

Left over issues are with content - dragging content out of a content window and dragging and dropping content into the content window.

Also, for the drag and drop shortcut part of this there's a work good around - you can drop into the address bar, then hit return to navigate.
Duplicate of this bug: 1092827
Can't drag even text from same tab to itself. Can test that here http://html5demos.com/drag-anything
Duplicate of this bug: 1096002
Duplicate of this bug: 1097212
Blocks: 1096030
Duplicate of this bug: 1101067
Blocks: 1103458
Duplicate of this bug: 1104945
Duplicate of this bug: 1103937
Blocks: e10s
Duplicate of this bug: 1109012
Duplicate of this bug: 1110087
smaug, we have a goal of nailing down all m4 bugs (of which this is one) by the new year. Is this bug still on your radar / are you actively working on it? If not, I'd like to volunteer to take it if you're ok with that.
Flags: needinfo?(bugs)
This is very much on my radar. Just tons of work. I'm pretty much doing only this now.
Should have a patch ready in couple of days.
child->parent/OS dnd works, child->child dnd works, and just implementing OS/parent->child.
(But I'm sure more edge cases come up when doing this. Our DND code is mostly 10+ years old.)
Flags: needinfo?(bugs)
Flags: firefox-backlog+
Posted patch wip v5 (obsolete) — Splinter Review
Attachment #8391557 - Attachment is obsolete: true
Posted patch wip6 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=a38516dcf660
(currently know to miss, 'drag' event, effectAllowed, dropEffect, dragImage)
Attachment #8540473 - Attachment is obsolete: true
Posted patch wip6.1 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=98c4b78f3e26
(this might compile on try without warnings)
Attachment #8540668 - Attachment is obsolete: true
Posted patch wip6.2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=7aca4b1b2224

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.com-7aca4b1b2224/

Not-working:
- Windows-only: dnd images from web pages to file manager (only link is copied)
- 'drag' event
- effectAllowed
- dropEffect
- dragImage
- OSX not tested at all

Working:
- File dnd from OS to web pages (<input type="file"> and other dataTransfer.files handling)
- most of the dnd events.
- in-page dnd
- dnd from browser chrome to content area
- dnd from content to browser chrome
- selection dragging and image dragging visualized
Attachment #8540689 - Attachment is obsolete: true
Duplicate of this bug: 1115535
Duplicate of this bug: 1115893
Posted patch v11 (obsolete) — Splinter Review
This let's one to drag images from child process to file manager also on Windows. (Linux works too, since that is where things are simpler anyway)

OSX totally untested.

Windows builds for v10, which is the same as v11
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.com-7caefc92e9f4/
Try is closed so could push to get also OSX builds.


Missing:
- 'drag' event
- effectAllowed
- dropEffect
- dragImage
Attachment #8540764 - Attachment is obsolete: true
actually, dragImage works already.
Posted patch v12 (obsolete) — Splinter Review
Some kind of initial review for this would be good.

drag event and dropEffect etc work now at least on linux.

https://tbpl.mozilla.org/?tree=Try&rev=0da59dc71ceb
Attachment #8548929 - Attachment is obsolete: true
Attachment #8549289 - Flags: review?(enndeakin)
Depends on: 1121947
(In reply to Olli Pettay [:smaug] from comment #49)
> Created attachment 8549289 [details] [diff] [review]
> drag event and dropEffect etc work now at least on linux.
Well, drop effect needs still some work, but I think that could be improved in followups.
(also, we seem to handle drop effect in different ways in different platforms)
I'm using with the last nightly build in Linux, and I don't have drag event.
Blocks: 1121946
Blocks: 1121947
No longer depends on: 1121947
(working on the drop effect still)
Is there an ETA on this besides it will be done when it's done?
Duplicate of this bug: 1123726
Duplicate of this bug: 1122525
Posted patch v15 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=eeb2f4597302

Couple of hacks less.
Attachment #8549289 - Attachment is obsolete: true
Attachment #8549289 - Flags: review?(enndeakin)
Attachment #8555478 - Flags: review?(enndeakin)
Posted patch v16 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=16aa519aefd2


(tiny change to try to handle better case when there are both remote and non-remote tabs in one window)
Attachment #8555478 - Attachment is obsolete: true
Attachment #8555478 - Flags: review?(enndeakin)
Attachment #8555482 - Flags: review?(enndeakin)
Posted patch v17 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c620faba8a07
Attachment #8555482 - Attachment is obsolete: true
Attachment #8555482 - Flags: review?(enndeakin)
Attachment #8555816 - Flags: review?(enndeakin)
Depends on: 1126902
Posted patch v18 (obsolete) — Splinter Review
Just tiny change to remove the hack for bug 1075670.
(but coordinates are still a bit wrong because of bug 1126902)

https://tbpl.mozilla.org/?tree=Try&rev=cae2c219cbb4
Attachment #8555816 - Attachment is obsolete: true
Attachment #8555816 - Flags: review?(enndeakin)
Attachment #8555958 - Flags: review?(enndeakin)
Comment on attachment 8555958 [details] [diff] [review]
v18

>diff --git a/dom/base/nsContentAreaDragDrop.cpp b/dom/base/nsContentAreaDragDrop.cpp
>-  if (isChromeShell && !editingElement)
>+  if (isChromeShell && !editingElement) {
>+    nsCOMPtr<nsIFrameLoaderOwner> flo = do_QueryInterface(mTarget);
>+    if (flo) {
>+      nsRefPtr<nsFrameLoader> fl = flo->GetFrameLoader();
>+      if (fl) {
>+        TabParent* tp = static_cast<TabParent*>(fl->GetRemoteBrowser());
>+        if (tp) {
>+          tp->AddInitialDnDDataTo(aDataTransfer);
>+        }
>+      }
>+    }
>     return NS_OK;
>+  }

It took me some time to figure out what was happening here. How you cache the initial data when the drag starts then grab it here needs some documentation.

However, my testing shows that Produce is called three times when a drag starts. Once in the parent, then in the child, and then in the parent again. I would expect it to only be called once per process.


>+void
>+nsContentUtils::TransferablesToIPCTransferables(nsISupportsArray* aTransferables,
>+                                                nsTArray<IPCDataTransfer>& aIPC,
>+                                                mozilla::dom::nsIContentChild* aChild,
>+                                                mozilla::dom::nsIContentParent* aParent)
>+{
>+            nsCOMPtr<nsISupports> data;
>+            uint32_t dataLen = 0;
>+            item->GetTransferData(flavorStr.get(), getter_AddRefs(data), &dataLen);
>+
>+            nsCOMPtr<nsISupportsString> text = do_QueryInterface(data);
>+            if (text) {
>+              nsAutoString data;
>+              text->GetData(data);

You reuse the variable name 'data' from only a few lines above. You do the same with 'count' earlier in this method.


>+    nsCOMPtr<nsIDragSession> dragSession = nsContentUtils::GetDragSession();
>+    uint32_t dropEffect = nsIDragService::DRAGDROP_ACTION_NONE;
>+    uint32_t action = nsIDragService::DRAGDROP_ACTION_NONE;
>+    if (dragSession) {
>+      dragSession->DragEventDispatchedToChildProcess();
>+      dragSession->GetDragAction(&action);
>+      nsCOMPtr<nsIDOMDataTransfer> initialDataTransfer;
>+      dragSession->GetDataTransfer(getter_AddRefs(initialDataTransfer));
>+      if (initialDataTransfer) {
>+        initialDataTransfer->GetDropEffectInt(&dropEffect);
>+      }
>+    }

Do you need the drop effect in case it was changed by a capturing chrome listener?


>+void
>+EventStateManager::BeginTrackingDragGesture(nsIContent* aContent)
>+{
>+  mGestureDownContent = aContent;
>+  mGestureDownFrameOwner = aContent;
>+}

I think it would be clearer if this method had a different name. The existing BeginTrackingDragGesture
is called on mousdown whereas this one you added would be called after the mouse has moved.


>@@ -3125,16 +3164,17 @@ EventStateManager::PostHandleEvent(nsPre
>       // that a drag is allowed. If the event isn't cancelled, a drop won't be
>       // allowed. Essentially, to allow a drop somewhere, specify the effects
>       // using the effectAllowed and dropEffect properties in a dragenter or
>       // dragover event and cancel the event. To not allow a drop somewhere,
>       // don't cancel the event or set the effectAllowed or dropEffect to
>       // "none". This way, if the event is just ignored, no drop will be
>       // allowed.
>       uint32_t dropEffect = nsIDragService::DRAGDROP_ACTION_NONE;
>+      uint32_t action = nsIDragService::DRAGDROP_ACTION_NONE;
>       if (nsEventStatus_eConsumeNoDefault == *aStatus) {
>         // if the event has a dataTransfer set, use it.
>         if (dragEvent->dataTransfer) {
>           // get the dataTransfer and the dropEffect that was set on it
>           dataTransfer = do_QueryInterface(dragEvent->dataTransfer);
>           dataTransfer->GetDropEffectInt(&dropEffect);
>         }
>         else {

Since you've moved the 'action' declaration, you might want to change the name of the other 'action' declared a few lines lower within the else block:

          uint32_t action;
          dragSession->GetDragAction(&action);


>+      if (ContentChild* child = ContentChild::GetSingleton()) {
>+        child->SendUpdateDropEffect(action, dropEffect);
>+      }
>+      if (dispatchedToContentProcess) {
>+        dragSession->SetCanDrop(true);
>+      }

Why doesn't ContentParent::RecvUpdateDropEffect set the drag action and the canDrop state in the
drag session in the same way as the parent does above? Why does this end up doing some Linux-specific code?


>-
>+public:
>   bool IsTrackingDragGesture ( ) const { return mGestureDownContent != nullptr; }
>+protected:

I'm guessing this change was something left in for debugging?


>+void
>+ContentParent::MaybeInvokeDragSession(TabParent* aParent)
>+{
>+  nsCOMPtr<nsIDragService> dragService =
>+    do_GetService("@mozilla.org/widget/dragservice;1");
>+  if (dragService && dragService->MaybeAddChildProcess(this)) {
>+    // We need to send transferrable data to child process.

transferrable -> transferable (with only one r)


>+    nsCOMPtr<nsIDragSession> session;
>+    dragService->GetCurrentSession(getter_AddRefs(session));
>+    if (session) {
>+      nsTArray<IPCDataTransfer> dataTransfers;
>+      nsCOMPtr<nsIDOMDataTransfer> domTransfer;
>+      session->GetDataTransfer(getter_AddRefs(domTransfer));
>+      nsCOMPtr<DataTransfer> transfer = do_QueryInterface(domTransfer);
>+      if (!transfer) {
>+        transfer = new DataTransfer(nullptr, NS_DRAGDROP_DROP, true, -1);
>+        session->SetDataTransfer(transfer);

Is this intended to only be called on a drop event? The call to the DataTransfer constructor here implies so, but this is being called within the dragover event.


>+      }
>+      transfer->FillAllExternalData();

I really don't like getting all of the data like this. When dragging large amounts of data, this can mean large amounts of info being requested from the source application and copied around several times and also possibly requiring the source application to convert all of the data from its own formats. On the drop event, it's almost always the case that only one type of data is needed. Wouldn't it be easier to just have the child ask the parent for the data on demand?

For other events (dragenter/dragover), remote sites can't actually access external data until the drop event (There's a security check just before the call to FillInExternalData), so getting all the data during these events just uses up time.

You also appear to not be getting the data again during the drop event. The data might also only be made available during the drop event, so you should also reget the data if needed.



>+void
>+TabParent::AddInitialDnDDataTo(DataTransfer* aDataTransfer)
...
>+      } else if (item.mType == DataTransferItem::DataType::eBlob) {
>+        variant->SetAsISupports(item.mBlobData);
>+      }
>+      aDataTransfer->SetDataWithPrincipal(NS_ConvertUTF8toUTF16(item.mFlavor),
>+                                          variant, i,
>+                                          nsContentUtils::GetSystemPrincipal());

You might want to comment that you're just using the system principal here rather than trying to get the right one. It doesn't matter for the spec, but we've always allowed access to data from the same source.


>       </handler>
>       <handler event="drop" group="system">
>       <![CDATA[
>-        if (!this.droppedLinkHandler || event.defaultPrevented)
>+        if (!this.droppedLinkHandler || event.defaultPrevented || this.isRemoteBrowser)
>           return;

Is this a temporary change and link dropping will be implemented later?

 
>@@ -1880,19 +1888,20 @@ nsDragService::RunScheduledTask()
>     // When the Xdnd protocol is used for source/destination communication (as
>     // should be the case with GTK source applications) a dragover event
>     // should have already been sent during the drag-motion signal, which
>     // would have already been received because XdndDrop messages do not
>     // contain a position.  However, we can't assume the same when the Motif
>     // protocol is used.
>     if (task == eDragTaskMotion || positionHasChanged) {
>         UpdateDragAction();
>+        TakeDragEventDispatchedToChildProcess(); // Clear the old value.
>         DispatchMotionEvents();
>-
>-        if (task == eDragTaskMotion) {
>+        if (!TakeDragEventDispatchedToChildProcess() &&
>+            task == eDragTaskMotion) {
>             // Reply to tell the source whether we can drop and what

I didn't look at these gtk changes in detail, as my previous comment indicated I wasn't sure why they were needed.


>+  for (uint32_t i = 0; i < mChildProcesses.Length(); ++i) {
>+    mozilla::unused << mChildProcesses[i]->SendEndDragSession(aDoneDrag);
>+  }

I'm guessing that dataTransfer.mozUserCancelled isn't supported by this.


>+  nsCOMPtr<nsIFrameLoaderOwner> flo = do_QueryInterface(dragNode);
>+  if (flo) {
>+    nsRefPtr<nsFrameLoader> fl = flo->GetFrameLoader();
>+    if (fl) {
>+      mozilla::dom::TabParent* tp =
>+        static_cast<mozilla::dom::TabParent*>(fl->GetRemoteBrowser());
>+      if (tp) {
>+        int32_t x, y;
>+        tp->TakeDragVisualization(*aSurface, x, y);
>+        if (*aSurface) {
>+          nsIFrame* f = fl->GetOwnerContent()->GetPrimaryFrame();
>+          if (f) {
>+            aScreenDragRect->x = x;
>+            aScreenDragRect->y = y;
>+            aScreenDragRect->width = (*aSurface)->GetSize().width;
>+            aScreenDragRect->height = (*aSurface)->GetSize().height;
>+          }
>+          return NS_OK;
>+        }
>+      }
>+    }
>+  }

What happens if some listener in the parent overrides the image by calling setDragImage? 

Does TabParent::mDnDVisualization stay around?


>+NS_IMETHODIMP
>+nsDragServiceProxy::InvokeDragSession(nsIDOMNode* aDOMNode,
>+                                      nsISupportsArray* aArrayTransferables,
>+                                      nsIScriptableRegion* aRegion,
>+                                      uint32_t aActionType)
...
>+  NS_ENSURE_STATE(doc->GetDocShell());
>+  mozilla::dom::TabChild* child =
>+    mozilla::dom::TabChild::GetFrom(doc->GetDocShell());
>+  nsTArray<mozilla::dom::IPCDataTransfer> dataTransfers;
>+  nsContentUtils::TransferablesToIPCTransferables(aArrayTransferables,
>+                                                  dataTransfers,
>+                                                  child->Manager(),
>+                                                  nullptr);

Null-check 'child'.


>+      bufLen = bufLen - map.mStride + (size.width * BytesPerPixel(format));
>+      nsDependentCString dragData(reinterpret_cast<char*>(map.mData), bufLen);
>+      mozilla::unused << child->SendInvokeDragSession(dataTransfers, aActionType, dragData,
>+                                                      size.width, size.height, map.mStride,
>+                                                      static_cast<uint8_t>(format),
>+                                                      dragRect.x, dragRect.y);

'dragData' here is the feedback image so should be named to indicate this, as 'dragData' to me implies the data of the drag not the image.
Similarly, for the argument names of SendInvokeDragSession.



Some other issues:

I assume the drag feedback image position being way off on Mac is the issue you described 59?


On the following test:

<html>
<div ondragstart="event.dataTransfer.setData('text/plain', 'test')" draggable="true">Drag Me</div>
<iframe src="http:///www.mozilla.com" width="300" height="300"/>
</html>

I get an assertion on drag:

###!!! ASSERTION: Bounds computation mismatch: 'mContainerBounds.IsEqualInterior(mAccumulatedChildBounds)', file /builds/moz2/working/layout/base/FrameLayerBuilder.cpp, line 3752


When I drop from a remote page to another application (such as the Terminal) I get:
 WARNING: No docshells for remote frames!: file /builds/moz2/working/dom/base/nsFrameLoader.cpp, line 524


You probably want some graphics person to review the drag image handling.
Attachment #8555958 - Flags: review?(enndeakin)
The patch hasn't been tested at all on OSX, but images and etc. what are being painted on parent process are now way off because bug 1075670 was backed out and 
bug 1126902 would cause them to be off at least on linux even if bug 1075670 was fixed.
WARNING: No docshells for remote frames!: file /builds/moz2/working/dom/base/nsFrameLoader.cpp, line 524
is just a warning we print way too often. Need to silence that.

The patch doesn't affect to layout, so that assertion isn't really related.
(In reply to Neil Deakin from comment #60)
> 
> It took me some time to figure out what was happening here. How you cache
> the initial data when the drag starts then grab it here needs some
> documentation.
Ok.



> 
> However, my testing shows that Produce is called three times when a drag
> starts. Once in the parent, then in the child, and then in the parent again.
> I would expect it to only be called once per process.
No, 3 times is ok. First we try to start the dnd in parent, since that is where events are dispatched first.
But there isn't anything drag there. Then events go to child process and we find something to drag, and then
parent can start its dnd.


> You reuse the variable name 'data' from only a few lines above. You do the
> same with 'count' earlier in this method.
Ok, will fix.


> >+    nsCOMPtr<nsIDragSession> dragSession = nsContentUtils::GetDragSession();
> >+    uint32_t dropEffect = nsIDragService::DRAGDROP_ACTION_NONE;
> >+    uint32_t action = nsIDragService::DRAGDROP_ACTION_NONE;
> >+    if (dragSession) {
> >+      dragSession->DragEventDispatchedToChildProcess();
> >+      dragSession->GetDragAction(&action);
> >+      nsCOMPtr<nsIDOMDataTransfer> initialDataTransfer;
> >+      dragSession->GetDataTransfer(getter_AddRefs(initialDataTransfer));
> >+      if (initialDataTransfer) {
> >+        initialDataTransfer->GetDropEffectInt(&dropEffect);
> >+      }
> >+    }
> 
> Do you need the drop effect in case it was changed by a capturing chrome
> listener?
Ah you mean non-initial dnd. Good question. I guess I should copy that.

> >+EventStateManager::BeginTrackingDragGesture(nsIContent* aContent)
> >+{
> >+  mGestureDownContent = aContent;
> >+  mGestureDownFrameOwner = aContent;
> >+}
> 
> I think it would be clearer if this method had a different name. The
> existing BeginTrackingDragGesture
> is called on mousdown whereas this one you added would be called after the
> mouse has moved.
Ok, will rename



> 
> Since you've moved the 'action' declaration, you might want to change the
> name of the other 'action' declared a few lines lower within the else block:
indeed

> 
> Why doesn't ContentParent::RecvUpdateDropEffect set the drag action and the
> canDrop state in the
> drag session in the same way as the parent does above? Why does this end up
> doing some Linux-specific code?
There isn't anything linux specific, except that UpdateDragEffect is implemented only for linux for now.
See the bugs blocking this.


> 
> I'm guessing this change was something left in for debugging?
Or some earlier version where IsTrackingDragGesture was needed.

> 
> >+    nsCOMPtr<nsIDragSession> session;
> >+    dragService->GetCurrentSession(getter_AddRefs(session));
> >+    if (session) {
> >+      nsTArray<IPCDataTransfer> dataTransfers;
> >+      nsCOMPtr<nsIDOMDataTransfer> domTransfer;
> >+      session->GetDataTransfer(getter_AddRefs(domTransfer));
> >+      nsCOMPtr<DataTransfer> transfer = do_QueryInterface(domTransfer);
> >+      if (!transfer) {
> >+        transfer = new DataTransfer(nullptr, NS_DRAGDROP_DROP, true, -1);
> >+        session->SetDataTransfer(transfer);
> 
> Is this intended to only be called on a drop event?
Not really. I it just that DataTransfer has silly ctor which depends on the event type.
I'll add a comment



> >+      transfer->FillAllExternalData();
> 
> I really don't like getting all of the data like this. When dragging large
> amounts of data, this can mean large amounts of info being requested from
> the source application and copied around several times and also possibly
> requiring the source application to convert all of the data from its own
> formats. On the drop event, it's almost always the case that only one type
> of data is needed. Wouldn't it be easier to just have the child ask the
> parent for the data on demand?
I'd say that is an optimization for a followup. There isn't usually that much
data to copy. Parent->Child blobs are lazy, and data is actually sent only if one
reads it in the child process.


> >       <handler event="drop" group="system">
> >       <![CDATA[
> >-        if (!this.droppedLinkHandler || event.defaultPrevented)
> >+        if (!this.droppedLinkHandler || event.defaultPrevented || this.isRemoteBrowser)
> >           return;
> 
> Is this a temporary change and link dropping will be implemented later?
link dropping works. It is implemented in 
embedding/browser/nsDocShellTreeOwner.cpp
That ends up using the same code what non-e10s uses for link dropping

> >+  for (uint32_t i = 0; i < mChildProcesses.Length(); ++i) {
> >+    mozilla::unused << mChildProcesses[i]->SendEndDragSession(aDoneDrag);
> >+  }
> 
> I'm guessing that dataTransfer.mozUserCancelled isn't supported by this.
We should probably remove mozUserCancelled... but ok, perhaps I'll copy it here for now.

> 
> What happens if some listener in the parent overrides the image by calling
> setDragImage? 
Ok, I need to check that.


> 
> Does TabParent::mDnDVisualization stay around?

No. there is
mDnDVisualization.forget();
Posted patch v19 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=3b8f4cf2d9ca

(this is now a bit hard to test because of that event.screenX/Y patch was backed out)
Attachment #8555958 - Attachment is obsolete: true
Attachment #8558797 - Flags: review?(enndeakin)
Duplicate of this bug: 1129150
Duplicate of this bug: 1131115
Duplicate of this bug: 1131881
(In reply to Olli Pettay [:smaug] (high review load) from comment #63)
> (In reply to Neil Deakin from comment #60)
> > 
> > It took me some time to figure out what was happening here. How you cache
> > the initial data when the drag starts then grab it here needs some
> > documentation.
> Ok.

Did you add this?


> No, 3 times is ok. First we try to start the dnd in parent, since that is
> where events are dispatched first.
> But there isn't anything drag there. Then events go to child process and we
> find something to drag, and then
> parent can start its dnd.

This seems to fire the dragstart event at the parent twice. 


> > I think it would be clearer if this method had a different name. The
> > existing BeginTrackingDragGesture
> > is called on mousdown whereas this one you added would be called after the
> > mouse has moved.
> Ok, will rename

I don't see this change.


> > I'm guessing this change was something left in for debugging?
> Or some earlier version where IsTrackingDragGesture was needed.

Did you want to revert the change?


> > 
> > >+    nsCOMPtr<nsIDragSession> session;
> > >+    dragService->GetCurrentSession(getter_AddRefs(session));
> > >+    if (session) {
> > >+      nsTArray<IPCDataTransfer> dataTransfers;
> > >+      nsCOMPtr<nsIDOMDataTransfer> domTransfer;
> > >+      session->GetDataTransfer(getter_AddRefs(domTransfer));
> > >+      nsCOMPtr<DataTransfer> transfer = do_QueryInterface(domTransfer);
> > >+      if (!transfer) {
> > >+        transfer = new DataTransfer(nullptr, NS_DRAGDROP_DROP, true, -1);
> > >+        session->SetDataTransfer(transfer);
> > 
> > Is this intended to only be called on a drop event?
> Not really. I it just that DataTransfer has silly ctor which depends on the
> event type.
> I'll add a comment

OK, it's a bit misleading to me since the drop event implies different access to the data.


> I'd say that is an optimization for a followup. There isn't usually that much
> data to copy. Parent->Child blobs are lazy, and data is actually sent only
> if one
> reads it in the child process.

I'm referring to the retrieving of the data from the source application which you're doing here regardless of whether the child uses it. This can block both applications until the data is available. I'm not clear why you need to get all the data upfront and can't instead just ask the parent for the data of the right type on demand within FillInExternalData.


> > 
> > Is this a temporary change and link dropping will be implemented later?
> link dropping works. It is implemented in 
> embedding/browser/nsDocShellTreeOwner.cpp
> That ends up using the same code what non-e10s uses for link dropping

OK, so the implementation there isn't the same, as it doesn't do the same security checks on urls. We should ensure they are identical or merge them. 


> > I'm guessing that dataTransfer.mozUserCancelled isn't supported by this.
> We should probably remove mozUserCancelled... but ok, perhaps I'll copy it
> here for now.

mozUserCancelled is needed for tab dragging.


> No. there is
> mDnDVisualization.forget();

I meant if the chrome parent overrides the image, does TabParent::mDnDVisualization get cleared?


You might want to verify with karlt about the gtk changes. They seem ok but I'm not 100% sure about the clearing of mTargetWidget/mTargetDragContext.
(In reply to Neil Deakin from comment #68)

> > I'd say that is an optimization for a followup. There isn't usually that much
> > data to copy. Parent->Child blobs are lazy, and data is actually sent only
> > if one
> > reads it in the child process.
> 
> I'm referring to the retrieving of the data from the source application
> which you're doing here regardless of whether the child uses it. This can
> block both applications until the data is available. I'm not clear why you
> need to get all the data upfront and can't instead just ask the parent for
> the data of the right type on demand within FillInExternalData.

I don't understand what you mean here.


> > > 
> > > Is this a temporary change and link dropping will be implemented later?
> > link dropping works. It is implemented in 
> > embedding/browser/nsDocShellTreeOwner.cpp
> > That ends up using the same code what non-e10s uses for link dropping
> 
> OK, so the implementation there isn't the same,
It is exactly the same code.

> as it doesn't do the same
> security checks on urls. We should ensure they are identical or merge them. 
That is then some other level of code.

> 
> mozUserCancelled is needed for tab dragging.
dragging tabs happens in the parent process only.


> I meant if the chrome parent overrides the image, does
> TabParent::mDnDVisualization get cleared?
Ah, I see. Yeah, it is possible mDnDVisualization stays there until the next dnd. Not a big deal though, but
can be cleared sure.
(In reply to Olli Pettay [:smaug] (high review load) from comment #69)
> > I'm referring to the retrieving of the data from the source application
> > which you're doing here regardless of whether the child uses it. This can
> > block both applications until the data is available. I'm not clear why you
> > need to get all the data upfront and can't instead just ask the parent for
> > the data of the right type on demand within FillInExternalData.
> 
> I don't understand what you mean here.
or I can, but not sure I see why it would be a real problem.
Files are read anyway lazily.
And for other cases it would be a bit annoying to make possibly several sync child-parent calls.
(so it is not clear which option is the best, send data, which usually is small, asap, or on-demand using sync messaging.)



I'm sure there will be tons of followup bugs after this. All of our dnd code is super messy, but need to get something
landed so that people can actually start using dnd in e10s.
(In reply to Neil Deakin from comment #68)
> This seems to fire the dragstart event at the parent twice. 
Because the first dragstart doesn't actually end up starting dnd unless chrome code wants to.
Comment on attachment 8558797 [details] [diff] [review]
v19

Karl, could you look at the gtk specific parts.
The idea is that we need to lazily update the drag action if we're dealing with
child process.

(OSX and Windows will be done in followup bugs.)
Attachment #8558797 - Flags: review?(karlt)
(In reply to Olli Pettay [:smaug] (high review load) from comment #70)
> > I don't understand what you mean here.
> or I can, but not sure I see why it would be a real problem.
> Files are read anyway lazily.
> And for other cases it would be a bit annoying to make possibly several sync
> child-parent calls.

You're doing this in your patch now by requiring a blocking call to get the data for each type. If you only get the data on demand, you only need to wait once since most uses should only use one type (and never at all if the user doesn't actually drop the data on the content window)

I'm pretty sure some edge cases will come up as well that will require you to not have cached the data.

But since remote tabs aren't enabled by default and if you're planning on working on followup bugs some more, yes, we should just check this in and see what happens.


> OK, so the implementation there isn't the same, It is exactly the same code.

The aDisallowInherit argument passed to dropLink is different in each case.


> Because the first dragstart doesn't actually end up starting dnd unless chrome code wants to.

I just tested this by adding some data within a chrome dragstart listener and the selection no longer works. Another followup bug to file here.
(In reply to Neil Deakin from comment #73) 
> > OK, so the implementation there isn't the same, It is exactly the same code.
> 
> The aDisallowInherit argument passed to dropLink is different in each case.
Ah, you mean that. The implementation is the same, but we may use it in a bit different way.
Looking.


> I just tested this by adding some data within a chrome dragstart listener
> and the selection no longer works.
That is the expected behavior in this e10s case - or at least the behavior I was aiming for.
So aDisallowInherit seems to be true only when drop happens in chrome.
I see, there is also the case in browser.xml, which passes true as last param, but embedding code
passes false. I'll change the embedding code.
Posted patch v20Splinter Review
Comments addressed 
(not making random flavors lazy though, need to get some measurements to show it actually makes sense.)
Attachment #8558797 - Attachment is obsolete: true
Attachment #8558797 - Flags: review?(karlt)
Attachment #8558797 - Flags: review?(enndeakin)
Attachment #8566680 - Flags: review?(karlt)
Attachment #8566680 - Flags: review?(enndeakin)
Comment on attachment 8566680 [details] [diff] [review]
v20

OK, so let's try it out then.
Attachment #8566680 - Flags: review?(enndeakin) → review+
Please put this out for testing.
Depends on: 1071562
(In reply to Neil Deakin from comment #68)
> I'm referring to the retrieving of the data from the source application
> which you're doing here regardless of whether the child uses it. This can
> block both applications until the data is available. I'm not clear why you
> need to get all the data upfront and can't instead just ask the parent for
> the data of the right type on demand within FillInExternalData.

I'm not advocating either way here but making a comment for future
consideration.

Yes, it would be preferable to fetch the data only when required and only of
the required type.  This makes sense for efficiency as there may be many types
available, but also ISTR some special types have quite different effects.

However, there is an intrinsic problem that much of the API is synchronous for
what is an async fetch.  Fetching the data before dispatching the motion or
drop events would allow the synchronous API to provide the data immediately
instead of spinning the event loop.
Comment on attachment 8566680 [details] [diff] [review]
v20

(In reply to Olli Pettay [:smaug] from comment #72)
> Karl, could you look at the gtk specific parts.

Just the changes in gtk/nsDragService.*?

> The idea is that we need to lazily update the drag action if we're dealing
> with child process.

That should be fine, but we need to ensure that gtk_drag_status() will be
called once and only once after task == eDragTaskMotion causes motion event
dispatch.

UpdateDragEvent() doesn't know whether the last task run was eDragTaskMotion.
It seems we need some state to track that so the method knows whether or not
to call ReplyToDragMotion().

Can we be sure that UpdateDragEvent() will be called (if no further drag events are received from the OS)?

ReplyToDragMotion() should be able to clear mTargetWidget and mTargetDragContext.  It would be good to do this to ensure there are no attempts to fetch data after the reply.

If UpdateDragEvent() has not been called when EndDragSession() is called, then
I guess that is due to ScheduleLeaveEvent() having been called, in which case there would be no need to call gtk_drag_status() (and so I think merely clearing mTargetWidget and mTargetDragContext is appropriate there).

>+  // Called when nsIDragSession implementation should update the UI for the
>+  // drag-and-drop based on the data got from the child process.
>+  void updateDragEffect();

Would it be accurate to say something like "based on the reply to a drag
motion event sent to a current target element in the child process"?
With GTK, this is the only opportunity to tell the source of the drag about
whether the drop would be accepted and if so what action will be taken.
If other platforms are different, that's probably fine so long as this call is
ignored with GTK when not appropriate.

Although I see async handling of motion event replies in
gtk/nsDragService.cpp, async replies to the drop event (for mCanDrop,
which depends on whether the drop event was cancelled, i assume) are not
handled here.

Similarly, gtk_drag_finish() should not be called after dispatching drop until
the destination has fetched the data in the desired format.  gtk_drag_finish()
is called before waiting for an async reply.  Are drop events requiring child
process communication handled synchronously?

Any issues in drop reply handling can be addressed later, I assume, but I'd like
to see us following the drag protocol correctly with gtk_drag_status() calls.
If this is holding up everything else, could the rest be landed now and the
gtk/nsDragService.cpp changes landed separately?

>@@ -1047,16 +1052,19 @@ nsDragService::ReplyToDragMotion()
>         // notify the dragger if we can drop
>         switch (mDragAction) {
>         case DRAGDROP_ACTION_COPY:
>           action = GDK_ACTION_COPY;
>           break;
>         case DRAGDROP_ACTION_LINK:
>           action = GDK_ACTION_LINK;
>           break;
>+        case DRAGDROP_ACTION_NONE:
>+          action = (GdkDragAction)0;
>+          break;
>         default:
>           action = GDK_ACTION_MOVE;
>           break;

Isn't there something wrong if mDragAction is NONE but mCanDrop is true?
Does that mean that the drag can drop, but nothing would happen?

>+  bool TakeDragEventDispatchedToChildProcess()
>+  {
>+    bool retval = mDragEventDispatchedToChildProcess;
>+    mDragEventDispatchedToChildProcess = false;
>+    return retval;
>+  }
>+

This method and/or mDragEventDispatchedToChildProcess would benefit from some
documentation about how, when, and for what purpose it is used.
Attachment #8566680 - Flags: review?(karlt)
Attachment #8566680 - Flags: review-
Attachment #8566680 - Flags: feedback+
(In reply to Karl Tomlinson (:karlt) from comment #81)
> Just the changes in gtk/nsDragService.*?
yes

> 
> > The idea is that we need to lazily update the drag action if we're dealing
> > with child process.
> 
> That should be fine, but we need to ensure that gtk_drag_status() will be
> called once and only once after task == eDragTaskMotion causes motion event
> dispatch.
Why only once?

> 
> UpdateDragEvent() doesn't know whether the last task run was eDragTaskMotion.
Right. Ok, I could add some flag there.


> Can we be sure that UpdateDragEvent() will be called (if no further drag
> events are received from the OS)?
No. child process can die or whatever


> 
> Would it be accurate to say something like "based on the reply to a drag
> motion event sent to a current target element in the child process"?
drag motion is some gtk specific thing, not web platform thing, so need to use some other terminology.


> Are drop events requiring child
> process communication handled synchronously?
I don't understand the question. There is no synchronous parent->child communication here.

> Isn't there something wrong if mDragAction is NONE but mCanDrop is true?
> Does that mean that the drag can drop, but nothing would happen?
Well, mCanDrop should be in case of e10s interpreted closer to mMayDrop, since we don't know
the answer before child tells us.

> 
> >+  bool TakeDragEventDispatchedToChildProcess()
> >+  {
> >+    bool retval = mDragEventDispatchedToChildProcess;
> >+    mDragEventDispatchedToChildProcess = false;
> >+    return retval;
> >+  }
> >+
> 
> This method and/or mDragEventDispatchedToChildProcess would benefit from some
> documentation about how, when, and for what purpose it is used.
Oh, I thought the naming was rather clear. At any point between two TakeDragEventDispatchedToChildProcess() calls
the return value of the method may change, and calling it resets the value. So it is up to the user to
think if events might have been dispatched to child process.
('Take*' is used in DOM and also in web platform for this kind of methods)
But I'll add something.
Posted patch v21Splinter Review
Attachment #8571672 - Flags: review?(karlt)
Comment on attachment 8571672 [details] [diff] [review]
v21

The use of mTargetDragContextForRemote looks good, thanks, provided there is
no need to fetch data after the dragover event dispatch has completed.

r+, if that is the case, with the following change, and some clarification in the documentation of updateDragEffect, please (see below).

>@@ -480,16 +480,21 @@ nsDragService::EndDragSession(bool aDone
>+    
>+    // We're done with the drag context.
>+    mTargetWidget = nullptr;
>+    mTargetDragContext = nullptr;

Please remove these lines now that these members are still cleared in
RunScheduledTask().

(In reply to Olli Pettay [:smaug] from comment #82)
> > That should be fine, but we need to ensure that gtk_drag_status() will be
> > called once and only once after task == eDragTaskMotion causes motion event
> > dispatch.
> Why only once?

The GTK documentation [1] may not be particularly clear about this, but the
protocol [2] is clear.  One and only one status message is sent in reply to a
position message.  The status message doesn't have an id to associate it with
a particular position message, and so an additional status message may get interpreted as a reply to a subsequent position message.

[1] https://developer.gnome.org/gtk3/3.14/GtkWidget.html#GtkWidget-drag-motion may
[2] http://www.newplanetsoftware.com/xdnd/

> > Can we be sure that UpdateDragEvent() will be called (if no further drag
> > events are received from the OS)?
> No. child process can die or whatever

I think the case of the child process dying should be fine.  It is not common
I hope and the source process will eventually time out its waiting.

However, having the drag stall on waiting for something that won't happen is
not something we want to happen regularly.  Some documentation to
indicate that updateDragEffect() should be called in response to each drag
NS_DRAGDROP_OVER message sent to the child would be helpful to indicate that
this must be called even when the effect does not change.

> > Would it be accurate to say something like "based on the reply to a drag
> > motion event sent to a current target element in the child process"?
> drag motion is some gtk specific thing, not web platform thing, so need to
> use some other terminology.

NS_DRAGDROP_OVER seems to be the corresponding name.

> > Are drop events requiring child
> > process communication handled synchronously?
> I don't understand the question. There is no synchronous parent->child
> communication here.

I was asking whether that was the mechanism to ensure that the reply to the
drop message was received before gtk_drag_finish() was called.
I understand there is currently no mechanism.

> > Isn't there something wrong if mDragAction is NONE but mCanDrop is true?
> > Does that mean that the drag can drop, but nothing would happen?
> Well, mCanDrop should be in case of e10s interpreted closer to mMayDrop,
> since we don't know the answer before child tells us.

Here, we don't need to know the answer before the child tells us.
ReplyToDragMotion() is reporting what the child has told us, so mCanDrop
should have the right value at this point.

I'm not clear why we have both canDrop and a dragAction none state on
nsIDragSession, but I don't see any need for canDrop to be true when
dragAction is none.
Attachment #8571672 - Flags: review?(karlt) → review+
(In reply to Karl Tomlinson (:karlt) from comment #84)
> Here, we don't need to know the answer before the child tells us.
> ReplyToDragMotion() is reporting what the child has told us, so mCanDrop
> should have the right value at this point.
We need to keep mCanDrop true when sending event to child.
and at that point we don't really know anything about possible action.
Posted patch v22Splinter Review
Improved the comments a bit, replaced the two null assignments with mTargetDragContextForRemote = null; (since the value might not be null there if child process dies)

https://tbpl.mozilla.org/?tree=Try&rev=c0b6595bd636
Blocks: 1075670
No longer blocks: 1075670
Depends on: 1075670
Blocks: 1071562
No longer depends on: 1071562
I can confirm this is a repeatable bug with Firefox 39.0a1 (2015-03-12) on OS X 10.6.8
Hey smaug - out of curiosity, what's this bug blocked on?
Flags: needinfo?(bugs)
Oh, nevermind - it looks like we're waiting on bug 1075670.
Flags: needinfo?(bugs)
Duplicate of this bug: 1146768
Bug 1075670 is now closed - are we all good to land this now?
Flags: needinfo?(bugs)
Did the merge happen already? I was thinking to land this after the merge.
Ah, yes - waiting until after the merge would probably be prudent. :) No, I don't believe the merges are done yet. My bad.
Flags: needinfo?(bugs)
Let's wait a day or so on this to see how bug 1149183 gets resolved.
Duplicate of this bug: 1151428
Bug 1075670 is actually fixed now. Ready to land, Olli?
Flags: needinfo?(bugs)
(merging to up-to-date tree)
Posted patch v24 (obsolete) — Splinter Review
...but looks like we have regressed something since dragging files from
file manager to <input type="file"> doesn't work anymore.
Debugging...
Attachment #8572030 - Attachment is obsolete: true
oh, seems to work after all. I guess my testcase was a bit silly :)
Flags: needinfo?(bugs)
https://hg.mozilla.org/integration/mozilla-inbound/rev/55524bdeb708

Crossing fingers, throwing salt over shoulders
(In reply to Olli Pettay [:smaug] from comment #100)
> oh, seems to work after all. I guess my testcase was a bit silly :)

It didn't work for me. Dropping files/links from other apps to content do not yet work with the latest inbound build.
Landed a trivial followup to add an "override" annotation, to make clang 3.6 & newer happy:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/33d37539c4ab
(In reply to Masatoshi Kimura [:emk] from comment #102) 
> It didn't work for me. Dropping files/links from other apps to content do
> not yet work with the latest inbound build.
WFM. Please file a new bug.
And please indicate which OS. I expect stuff to be most broken on OSX.
Hm, it suddenly become working for me. Maybe intermittent failure? I'll file a bug when I reproduced the issue again.
Backed out for being my best guess as to what caused frequent m-e10s asserts on Linux debug. Sorry, it landed at a time when the suite was completely busted due to another push, so I don't have firm results from the actual push. However, this was the only suspect in the range that didn't have a recent Try run as far as I could tell.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfea20361c1e

https://treeherder.mozilla.org/logviewer.html#?job_id=8603139&repo=mozilla-inbound
[when this re-lands, please feel free/encouraged to just fold my followup from comment 103 into the main patch.]
tn>	smaug: yeah, we get that assertion sometimes. i dont think anyone has ever really looked into it.
Posted patch v25Splinter Review
Waiting for tryserver results for this.
Attachment #8589684 - Attachment is obsolete: true
The inbound version seems to work fine on Windows 8.1.  Also the add-on Super Drag works as well.
https://hg.mozilla.org/mozilla-central/rev/1eec2c8789c1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Iteration: --- → 40.1 - 13 Apr
Flags: qe-verify?
Depends on: 1152888
Depends on: 1153613
Definitely not fixed for me; https://hg.mozilla.org/mozilla-central/rev/2c9708e6b54d, win 7x64 (32 bit build).

Filing a new bug.
Depends on: 1153878
Depends on: 1156703
Depends on: 1160043
No longer blocks: 1095881
Duplicate of this bug: 1095881
Duplicate of this bug: 1111962
Depends on: 1233612
Depends on: 1244533
Depends on: 1248140
Depends on: 1254085
Depends on: 1376369
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.