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

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

To be more clear, this includes attempting to drag links, text, images, and tabs from or to an e10s-enabled browser

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

In Linux, attempting to use drag and drop just crashes. We should at least fix that.

Comment 3

[Tom S [:evilpie]]

Attachment: test

Comment 4

[Tom S [:evilpie]]

Attachment: wip

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.

Comment 7

[Tom S [:evilpie]]

This bug is too big for me to tackle at the moment, pick it up if you want.

Comment 10

[Chris Peterson [:cpeterson]]

smaug volunteered to investigate drag-and-drop on Linux.

Comment 11

[Chris Peterson [:cpeterson]]

Move old M2 P3 bugs to M4 (because tracking-e10s=m5+ flag doesn't exist yet).

Comment 16

[Chris Peterson [:cpeterson]]

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.

Comment 17

[Neil Deakin]

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.

Comment 18

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

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.)

Comment 19

[Chris Peterson [:cpeterson]]

This bug is not related to dragging browser tabs. For those problems, see bug 1012784 or bug 1068167.

Comment 20

[Jim Mathies [:jimm]]

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.

Comment 21

[Jim Mathies [:jimm]]

Olli, are you currently working on this?

Comment 22

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

Yes. Was my earlier comment not clear enough? :)

Comment 23

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

(I don't see why this would block clipboard)

Comment 27

[Jim Mathies [:jimm]]

(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.

Comment 29

[Dāvis]

Can't drag even text from same tab to itself. Can test that here http://html5demos.com/drag-anything

Comment 38

[Jim Mathies [:jimm]]

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.

Comment 39

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

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.)

Comment 40

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

Attachment: wip v5

Comment 41

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

Attachment: wip6

https://tbpl.mozilla.org/?tree=Try&rev=a38516dcf660
(currently know to miss, 'drag' event, effectAllowed, dropEffect, dragImage)

Comment 42

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

Attachment: wip6.1

https://tbpl.mozilla.org/?tree=Try&rev=98c4b78f3e26
(this might compile on try without warnings)

Comment 43

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

Attachment: wip6.2

https://tbpl.mozilla.org/?tree=Try&rev=7aca4b1b2224

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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

Comment 46

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

Attachment: v11

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/[e-mail]-7caefc92e9f4/
Try is closed so could push to get also OSX builds.


Missing:
- 'drag' event
- effectAllowed
- dropEffect
- dragImage

Comment 47

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

Now with other platforms too, https://tbpl.mozilla.org/?tree=Try&rev=54f710b84dcd
Will be in http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/

Comment 48

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

actually, dragImage works already.

Comment 49

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

Attachment: v12

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

Comment 50

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

(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)

Comment 51

[Yakir Sitbon]

I'm using with the last nightly build in Linux, and I don't have drag event.

Comment 52

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

(working on the drop effect still)

Comment 53

[Gary [:streetwolf52]]

Is there an ETA on this besides it will be done when it's done?

Comment 56

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

Attachment: v15

https://tbpl.mozilla.org/?tree=Try&rev=eeb2f4597302

Couple of hacks less.

Comment 57

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

Attachment: v16

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)

Comment 58

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

Attachment: v17

https://tbpl.mozilla.org/?tree=Try&rev=c620faba8a07

Comment 59

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

Attachment: v18

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

Comment 60

[Neil Deakin]

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.

Comment 61

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

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.

Comment 62

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

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.

Comment 63

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

(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();

Comment 64

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

Attachment: v19

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)

Comment 68

[Neil Deakin]

(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.

Comment 69

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

(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.

Comment 70

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

(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.

Comment 71

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

(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 72

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

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.)

Comment 73

[Neil Deakin]

(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.

Comment 74

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

(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.

Comment 75

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

So aDisallowInherit seems to be true only when drop happens in chrome.

Comment 76

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

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.

Comment 77

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

Attachment: v20

Comments addressed 
(not making random flavors lazy though, need to get some measurements to show it actually makes sense.)

Comment 78

[Neil Deakin]

Comment on attachment 8566680 [details] [diff] [review]
v20

OK, so let's try it out then.

Comment 79

[Gary [:streetwolf52]]

Please put this out for testing.

Comment 80

[Karl Tomlinson (:karlt)]

(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 81

[Karl Tomlinson (:karlt)]

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.

Comment 82

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

(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.

Comment 83

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

Attachment: v21

Comment 84

[Karl Tomlinson (: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.

Comment 85

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

(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.

Comment 86

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

Attachment: v22

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

Comment 87

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

Attachment: v23

https://tbpl.mozilla.org/?tree=Try&rev=7eab31cce3bd

Comment 88

[indigoglow]

I can confirm this is a repeatable bug with Firefox 39.0a1 (2015-03-12) on OS X 10.6.8

Comment 89

[Mike Conley (:mconley) (:⚙️)]

Hey smaug - out of curiosity, what's this bug blocked on?

Comment 90

[Mike Conley (:mconley) (:⚙️)]

Oh, nevermind - it looks like we're waiting on bug 1075670.

Comment 92

[Mike Conley (:mconley) (:⚙️)]

Bug 1075670 is now closed - are we all good to land this now?

Comment 93

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

Did the merge happen already? I was thinking to land this after the merge.

Comment 94

[Mike Conley (:mconley) (:⚙️)]

Ah, yes - waiting until after the merge would probably be prudent. :) No, I don't believe the merges are done yet. My bad.

Comment 95

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Let's wait a day or so on this to see how bug 1149183 gets resolved.

Comment 97

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Bug 1075670 is actually fixed now. Ready to land, Olli?

Comment 98

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

(merging to up-to-date tree)

Comment 99

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

Attachment: v24

...but looks like we have regressed something since dragging files from
file manager to <input type="file"> doesn't work anymore.
Debugging...

Comment 100

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

oh, seems to work after all. I guess my testcase was a bit silly :)

Comment 101

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/55524bdeb708

Crossing fingers, throwing salt over shoulders

Comment 102

[Masatoshi Kimura [:emk]]

(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.

Comment 103

[Daniel Holbert [:dholbert]]

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

Comment 104

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

(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.

Comment 105

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

And please indicate which OS. I expect stuff to be most broken on OSX.

Comment 106

[Masatoshi Kimura [:emk]]

Hm, it suddenly become working for me. Maybe intermittent failure? I'll file a bug when I reproduced the issue again.

Comment 107

[Ryan VanderMeulen [:RyanVM]]

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

Comment 108

[Daniel Holbert [:dholbert]]

[when this re-lands, please feel free/encouraged to just fold my followup from comment 103 into the main patch.]

Comment 109

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

tn>	smaug: yeah, we get that assertion sometimes. i dont think anyone has ever really looked into it.

Comment 110

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

Attachment: v25

Waiting for tryserver results for this.

Comment 111

[Gary [:streetwolf52]]

The inbound version seems to work fine on Windows 8.1.  Also the add-on Super Drag works as well.

Comment 112

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

Try looked better with v25
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eec2c8789c1

Comment 113

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eec2c8789c1

Comment 114

[Honza Bambas (:mayhemer)]

Definitely not fixed for me; https://hg.mozilla.org/mozilla-central/rev/2c9708e6b54d, win 7x64 (32 bit build).

Filing a new bug.