203307 - Implement drag/drop via IStream interface

Status: RESOLVED FIXED

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

Description

[Nisheeth Ranjan]

See bug 97413 for background info.

Implement drag drop via the
CF_FILEDESCRIPTOR and CF_FILECONTENT clipboard formats and pass the file's data
to the drop target through the IStream interface.  We'll need to serve the 
dragged file through a Necko stream to an IStream that the drop target will 
pull its data from.

Comment 1

[Scott MacGregor]

cc'ing myself. We need this now that Bug #83803 is fixed which added the ability
to drag mail attachments to the desktop. 

This fails miserably for imap where we have to wait for the attachment to be
downloaded as we spool it to the temp file. If the user releases the drop, the
OS trys to copy the temp file while we are busy writing to it. 

The solution proposed here is how 4.x worked and that will allow things to work
for imap drag and drop.

Comment 2

[Scott MacGregor]

Useful links:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/programmersguide/shell_basics/shell_basics_programming/transferring/clipboard.asp

iStream declaration:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/stg/stg/istream.asp

Comment 3

[Pit Müller]

Following Clipboard (Drag&Drop) Formats should be supported:

"FileGroupDescriptor"
"FileContents"

Comment 4

[Scott MacGregor]

Feel free to take back if you are still interested in doing this Nisheeth...

Comment 5

[Yuri Kovalchuk]

Attachment: Suggestion on IStream implementation.

Hi all!

I've searche alot before found (thanks to Ere Maijala) this bug report. It
seems like it is the only possible solution for large files. Also if we use
current implementation of nsDragService::StartInvokingDragSession (where
::DoDragDrop gets called) it won't help much. By default drag and drop
operation on windows is synchronous, only on Windows 2000 an attempt was made
to fix that issue. Microsoft provided IAsyncOperation interface that must be
exposed by data object to have data extraction take place on a separate thread.

I'm currently working on implementing file dragging from Mozilla to Windows
file system and have somewhat working implementation of an IStream interface
that solves the issue, i do not want to post it yet because it is not completly
functional. The only method that needs implemntation is IStream::Read - as I've
figured Shell does not call any other methods. And I think there is no need to
implement them either since the only purpose of this object is to download data
from internet to user's computer...(or something like that).
So if anyone is still working on this bug please contact me so that we could
find some good solution to this problem.

Thank you!
Yuri Kovalchuk

Comment 6

[Yuri Kovalchuk]

Attachment: Implements dnd via IStream

This is basically the same patch as my latest on for bug 267426 but with definitions for IAsyncOperation and related stuff. This should also fix mingw build failure (bug 344504).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 233462 [details] [diff] [review]
Implements dnd via IStream

ok, cool

Comment 8

[timeless]

Comment on attachment 233462 [details] [diff] [review]
Implements dnd via IStream

mozilla/widget/src/windows/nsDataObj.h 	1.36
mozilla/widget/src/windows/nsDataObj.cpp 	1.82
mozilla/widget/src/windows/nsDragService.cpp 	1.52
mozilla/widget/src/windows/nsDragService.h 	1.27

Comment 9

[Yuri Kovalchuk]

help with testing please

Comment 10

[juan becerra [:juanb]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060815 Minefield/3.0a1

Tested this with a 4.5mb image file (steves-digicams.com), and I was able to drag and drop onto the desktop and also a removable drive after clearing the cache.

Letting it simmer because at least one more person experienced a crash while testing this.

Comment 11

[David G King]

This seems to have given me the following build error on WinXP SP2/MinGW/cygwin

e:/mozilla/source/mozilla/widget/src/windows/nsDragService.cpp: In member functi
on `PRUint64 nsDragService::GetShellVersion()':
e:/mozilla/source/mozilla/widget/src/windows/nsDragService.cpp:519: error: `majL
L' was not declared in this scope
e:/mozilla/source/mozilla/widget/src/windows/nsDragService.cpp:519: error: `minL
L' was not declared in this scope
e:/mozilla/source/mozilla/widget/src/windows/nsDragService.cpp:517: warning: unu
sed variable 'maji64'
e:/mozilla/source/mozilla/widget/src/windows/nsDragService.cpp:518: warning: unu
sed variable 'mini64'
e:/mozilla/source/mozilla/widget/src/windows/nsDragService.cpp:519: warning: unu
sed variable 'majLL'
e:/mozilla/source/mozilla/widget/src/windows/nsDragService.cpp:519: warning: unu
sed variable 'minLL'
make[6]: *** [nsDragService.o] Error 1

Comment 12

[timeless]

Attachment: this should do it

Comment 13

[David G King]

Yup, seems to work fine.

Comment 14

[Yuri Kovalchuk]

Attachment: Patch for MOZILLA_1_8_BRANCH

This if against MOZILLA_1_8_BRANCH, please r & sr.
How do I make it go into MOZILLA_1_8_BRANCH?
What are these approval1.8.0.7, approval1.8.0.8 and approval1.8.1? Do I need them at all?
Looks like too many questions... oh well. At least help with the first one please.

Comment 15

[Frank Wein [:mcsmurf]]

You would need to request approval1.8.1 when it has been reviewed. I don't know what the chances are to get it on the 1.8 branch though since it's a quite big patch. When requesting approval, outline what the patch is needed for, what possible risk is involved with this patch and what testing is needed for verify the bug fix (if needed).

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 234197 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

rubber-stamp since this is very much the same as the trunk patch

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we're going to want this on the 1.8 branch for FF2 at this stage, probably, but you can request blocking status for this bug if you like. We definitely won't want this for 1.8.0 branch.

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 234197 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

Its too late at this time to take large patches to provide new implementions or new features.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Is bug 336337 the only reason this hasn't been checked in and marked FIXED?

Comment 20

[Yuri Kovalchuk]

(In reply to comment #19)
It is checked in on trunk. I tried to get it to Firefox 2.0 (MOZILLA_1_8_BRANCH) but it was too late for a patch this big :(
Timeless checked this in a while ago, but I forgot to mark it fixed, doing it now.

Comment 21

[Worcester12345]

After a "bake" period, can it block an update on 2.0 such as 2.0.1?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

No.

Comment 23

[Jay Patel [:jay]]

This patch looks a bit risky for the 1.8 branch, but we would still like to fix bug 245861.  Can we come up with a simpler patch to address the issue in that bug?

Comment 24

[Scott MacGregor]

Comment on attachment 234197 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

This has now been baking on the trunk since 08/15. Do we feel the risk level is better or still too risky? 

It fixes Firefox Bug 245861 which there has been interest in getting fixed on the branch.

It also makes drag and drop of mail attachments  to the desktop in Thunderbird work much better on Windows. Currently you have to hold the action until we finish downloading the attachment then you can release the drop. With this patch you can drop it right away and we stream it to the destination. Hence my interest in asking again :).

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 234197 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 234197 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

mscott has gotten cold feet on this, removing approval1.8.1.4 for now while trunk regressions are investigated.

Comment 27

[Yuri Kovalchuk]

(In reply to comment #26)
> trunk regressions are investigated.

This could be related to bug #358657 perhaps. I used to investigate this issue. Hope this helps.

Comment 29

[Wayne Mery (:wsmwk)]

Veditz in comment #26
> (From update of attachment 234197 [details] [diff] [review])
> mscott has gotten cold feet on this, removing approval1.8.1.4 for now while
> trunk regressions are investigated.

what bug# are thought to be regressions?

Comment 30

[Wayne Mery (:wsmwk)]

based on comment 1 this might also fix bug 253711, bug 383893, bug 283452, bug 384590

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #30)
> based on comment 1 this might also fix bug 253711, bug 383893, bug 283452, bug
> 384590

I don't think attachment 234197 [details] [diff] [review] fixes these bugs. These bugs need the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=358657#c16 too.