494989 - windows drag and drop does not use temporary file when needed--fixes dragging image to apps that can handle images

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Phil Lacy]

Attachment: for preliminary review and further testing advise

This code for deleting a temp file can occur when the windows o/s is still using it under async conditions.

I'd like someone to test this where x-moz-file is dragged outside the app.

If there is a scenerio in TB please advise by comment so it can be tested.  Or if other products use this, we need a test of this bug.

Comment 1

[Phil Lacy]

drag an image to an app such as mspaint and nothing happens.  Dragging data from moz app only utilizes streams and can not be read by external apps because the moz internal mechanisim of using CF_HDROP and temporary files and then deleting them is broke.

Comment 2

[Phil Lacy]

Attachment: email message for testing

this eml file should be usable to test dragging the image to mspaint.

Comment 3

[Phil Lacy]

Attachment: fix using and deleting temporary files.

this corrects ability to drag image to an external app.  deleting temp files can't be done in the nsDataObject passed with drag.  External apps do not open it before dismissing the drag operation and the CFSTR_PERFORMEDDROPEFFECT isn't sent by external apps.
This is similar to OExpress where the file is valid until the mail window is closed where the external app usually notifies the user that the file is not valid and it gives the option to save or not.

Another bug and patch will be submitted for utilizing the filepromise in the same way for text editors or pdf viewers.

And a followup patch will be submitted on another bug for x-moz-file to actually drag a file.

Comment 4

[Phil Lacy]

ref bug 245861 and bug 203307 for background.

Comment 5

[Ere Maijala (slow)]

Comment on attachment 382231 [details] [diff] [review]
fix using and deleting temporary files.

Please make mTemporaryFiles an nsCOMArray (see https://developer.mozilla.org/en/XPCOM_array_guide).

Comment 6

[Phil Lacy]

Attachment: uses nsCOMArray<nsIFile> per comments

This simplifies it, maybe it could have passed through the nsDataObject creation but right before the windows drop code seemed safer.  That way if there is future clipboard requirements which uses the same nsDataObject then it can be done there and then.

Comment 7

[Phil Lacy]

Comment on attachment 382425 [details] [diff] [review]
uses nsCOMArray<nsIFile> per comments

>diff --git a/widget/src/windows/nsClipboard.cpp b/widget/src/windows/nsClipboard.cpp
-1, TYMED_ISTREAM)
>         dObj->AddDataFlavor(kFilePromiseMime, &filedataFE);  
>+        SET_FORMATETC(filedataFE, CF_HDROP, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL)
>+        dObj->AddDataFlavor(kFilePromiseMime, &filedataFE);      
>         SET_FORMATETC(filedataFE, 

This is part of my Bug 484667 patch I'm working on and shouldn't be needed for this patch.

Comment 8

[Phil Lacy]

Comment on attachment 382425 [details] [diff] [review]
uses nsCOMArray<nsIFile> per comments

sorry, that whole first hunk is part if Bug 491239 and it wasn't intended for this bug to depend on that bug.  I'll get that removed, retest and resubmit.

Comment 9

[Phil Lacy]

Attachment: d&d of images to apps and delete temp file

This is patch on tip so it does not depend on other bugs.
Added test for null pointer and GetFile exits without handling temp files.

Comment 10

[Phil Lacy]

Comment on attachment 382622 [details] [diff] [review]
d&d of images to apps and delete temp file

the tempfile holder can't be in nsDataObject but it may not belong in dragservice.  It should be in the clipboard, let me check further to see if it can live a long happy life there

Comment 11

[Phil Lacy]

Attachment: add file holding array to clipboard

The clipboard object is separate from the dragservice.  The dragservice calls a helper function in nsClipboard.cpp but they will hold separate arrays and separate nsDataObj.
It appears we don't want the IDataObj to hold any type of reference to the array when the moz app along with its dragservice and clipboard objects are destroyed.

Comment 12

[Ere Maijala (slow)]

Comment on attachment 383050 [details] [diff] [review]
add file holding array to clipboard

r+ with these fixed:

nsDragService constructor is missing initializer for mTemporaryFiles. 

+    nsCOMArray<nsIFile> * mTemporaryFiles; // Owned by nsDragService for removing temp files

Owned by nsClipboard now, right?

Comment 13

[Phil Lacy]

Actually both, the nsClipboard and nsDragService can hold their own iDataObject's.
I didn't switch the temporary array to Clipboard I just gave it its own. They can't get them mixed up. They can both exist together since they are set in separate IDataObject's.

Clipboard registers or initializes it with windows with OleSetClipboard(dataObj) and that dataobj is owned by clipboard.
The dragservice uses DoDragDrop to register with windows and that dataObj is owned by dragService.

The clipboard has its own nsCOMPtr for its tempfiles.

Comment 14

[Phil Lacy]

Attachment: added comment

Need rereview because not adding all previous r+ comments.  See my previous comment.

Comment 15

[Phil Lacy]

windows

Comment 16

[Nochum Sossonko [:Natch]]

Is this a regression? Can you provide information on why it should block? Set the blocking-1.9.1.1 flag to '?' if you think it's a maintenance-release blocker, 1.9.1 is out the door already.

Comment 17

[Ere Maijala (slow)]

Comment on attachment 384267 [details] [diff] [review]
added comment

r+ with one more nit fixed:

+nsresult nsDataObj::SetTemporaryFiles(nsCOMArray<nsIFile>& aTemporaryFiles) {

The opening curly bracket should be on its own line.

Comment 18

[Phil Lacy]

Attachment: fixes final nit on brace formatting consistency

Comment 19

[Phil Lacy]

Attachment: fixes final nit on brace formatting consistency

Comment 20

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

Does this mean that these temporary files are only deleted when Firefox exits?

Comment 21

[Phil Lacy]

my testing has been with TB3 comm-central moz191. The files are deleted when Shredder exits.

Apparently outside programs terminate the drop before reading the file and any kind of deletion done in the dataobject is premature.

Comment 22

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

That's pretty bad. Is it true that all Windows apps have to leave these files around until they exit? That seems like incredibly bad API design.

Comment 23

[Phil Lacy]

IE7: open a file:/// and drag an image to mspaint and the file is still in it's temp location until you close IE7. IE7 won't allow you drag an image from the internet and if you drag to the desktop it asks you if you want to drag a file from "this zone"

OEXP. if you click open an email you can drag the attachments where ever but the temp file exists until you close the message window. you can't drag an image to mspaint from OEXP.  I'm about 80% sure that is relatively new. I believe I was able to drag an image out but I may have been mistaken for the dragging of attachment files.

If I could find a way to drag something from the main OExp window it probably would keep the temp file as well until that window is closed.

I'll see if I can investigate other apps

Comment 24

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

One problem with leaving temporary files around this long is that if we crash we are quite likely to leak temporary files. But I have no idea what we can do about that, if indeed we have to leave files open for the lifetime of the app.

Comment 25

[Rob Arnold [:robarnold]]

(In reply to comment #24)
> One problem with leaving temporary files around this long is that if we crash
> we are quite likely to leak temporary files. But I have no idea what we can do
> about that, if indeed we have to leave files open for the lifetime of the app.

I think that setting the delete-on-close flag on the file handle when we make it will avoid this issue.

Comment 26

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

And leave it open for the lifetime of the app? Won't we effectively be leaking file handles?

> if indeed we have to leave files open for the lifetime of the app.

I misspoke here, with this patch we only leave files lying around for the lifetime of the app, they're not leaking.

Comment 27

[Phil Lacy]

IE and OExp, uses Temporary Internet Files\content.IE5 that is hidden and not easily accessible by explorer.

We could come up with our own temp folder and delete the contents when we start the Moz app.

A short timer would probably be enough. The apps I experienced send an alert the dropped file no longer exists and prompts to close or save the data as file.

Comment 28

[Phil Lacy]

Comment on attachment 388398 [details] [diff] [review]
fixes final nit on brace formatting consistency

https://bugzilla.mozilla.org/attachment.cgi?id=388398&action=diff#a/widget/src/windows/nsDataObj.cpp_sec3

nsDataObj::GetFile needs to be reformatted, I'm going to reformat the function and submit a bug on it, then get back to this problem and using getfile for kfilepromise and kfilemime.

Comment 29

[Phil Lacy]

Attachment: for review. timer and temp file limit

For roc to look over and see if this timer would be acceptable.

The timer Sleep(100) takes care of dragging an image to mspaint. We may be able to go back to just a single cached file to pass in to the dataobj and do a Sleep and then delete the cached temp file but if we leave in the file array then maybe it will come in handy for multiple file d&d (which doesn't currently work, see other bug).

This has some reformatting because of braces, {}, which makes a diff of a lot of unchanged code. I can submit another patch without the white space changes for clarity if needed.

After further direction I can clean it up for formal r and sr.

Comment 30

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

Does this hang the browser for 100ms?

A synchronous sleep here seems like a bad idea and 100ms doesn't seem long enough anyway. I'd be much happier with setting up a timer event to fire after, say, 10 seconds, so the other app has more time and so that we don't block.

Comment 31

[Phil Lacy]

So would we need an array of arrays of temp files? I don't know how synchronous dragservice is but I assume other drags of single or multi files will occur. I figure the application-scoped single array would have an indeterminate number and sequence of temp files loaded at point in time. So one drag may not have a sequential set of files in the array.

nsDragObj is set up for dnd of one item but windows expects it to handle multi item. So I see that this may be up for a rewrite sometime in the future to load up the nsDataObj with multi items.

Or are you thinking we just need to give the windows drop mechanism a single nsIFile ptr to use and then we can pass that off to a timer event and change it when we implement multi-item dnd.

Do we have a timer event to use that will survive the application? It can be expected to close before dnd downloads are done.

Comment 32

[Rob Arnold [:robarnold]]

(In reply to comment #31)
> Do we have a timer event to use that will survive the application? It can be
> expected to close before dnd downloads are done.

No. You could observe the xpcom-shutdown event and reopen the file with 'delete on close'. If the user is shutting down the application, I don't think it's unreasonable for us to clean up these temp files. We could also schedule the OS to delete them on the next restart.

Comment 33

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

You could create one array of temporary files per drag-and-drop operation and create a timer per operation also, associating the array with the timer. No global array should be needed.

Comment 34

[Phil Lacy]

Attachment: nsITimer in with nsDataObj

I didn't use the array. That would just confuse this object since it can't do multi anything dragging outside the moz app.
I'm going to do some more testing but since this is my first try at a time I better have a review to see if I got all the sanity checks needed, but it is working in preliminary tests of small images dragged to mspaint.

Comment 35

[Phil Lacy]

Comment on attachment 399404 [details] [diff] [review]
nsITimer in with nsDataObj

>+void nsDataObj::RemoveTempFile(nsITimer* aTimer, void* aClosure)
>+{
>+  nsDataObj *timedDataObj = static_cast<nsDataObj *>(aClosure);
>+  if (timedDataObj) 
>+    timedDataObj->mCachedTempFile->Remove(PR_FALSE);
>+  timedDataObj->mCachedTempFile = NULL;
>+  timedDataObj->Release();

I see this needed to be
+  if (timedDataObj->mCachedTempFile) 

T'was checked once, but I don't know what may happen in the timer's 10 seconds.

Comment 36

[Phil Lacy]

500 ms is plenty. If the app is closed before the timer fires then the temp file is not deleted.

We need to put this type of transfers external to the app in a more autonomous window like the download window.

This patch can still land if it gets reviewed, if we can see fit to put it into the 1.9.1.x branch then TB3 can use it.

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You (Phil) seem to have multiple bugs going for enhancements to the windows drag and drop code, but I'm going to post all of my comments here.  I've been working for a couple weeks in Bug 338478 to enhance nsDataObj and nsDataObjCollection to handle CF_HDROPs properly without realizing that you had other bugs open on this, but I think that you are going down the wrong road here.

1) As I said in your bug about the download manager proper, this does not appear to be built against mozilla-central.

2) This patch is particular worries me. The current implementation of CF_HDROP is already a kludgy hack to get Photoshop to work.  The true bug in our code is that we are advertising that we support CF_HDROP while we violate one of the fundamental assumptions, namely that the file actually exists on disk.  By advertising a file with the CF_HDROP type we claiming that there is a file that exists on disk independent of whether firefox is running.  There is no reason that Paint (or some other app) can't wait 30 seconds before trying to load the data.  By advertising a CF_HDROP, we've stated that the file exists on disk for the receiving application to pick up at its leisure.

3) In other bugs, you've modified nsDataObj::GetFile() to support application/x-moz-file-promise when there is a comment that states at the top of the function why this is not possible.

The operative MSDN quote "This clipboard format [CF_HDROP] is used when transferring the locations of a group of existing files."  Any and all temporary files should be passed under the CFSTR_FILEDESCRIPTOR/CFSTR_FILECONTENTS formats (corresponding to the mime type application/x-moz-file-promise) which I believe Paint supports just fine (don't have time to test though).

Basically, I think hacking around a fundamental assumption of this clipboard format with timers and whatnot is a very bad idea.

Comment 38

[Phil Lacy]

I got the patches updated to m-c and will send them up this weekend.

There's a few non sequiturs in nsDataObj which I noted throughout my patches. Notably bug 491239 sets the promisefile and kfilemime to cf_cdrop but it isn't in GetFile() and that caused a freeze condition which the drivers ended up accepting my hack to correct the freeze but the bug is still there.


(In reply to comment #37)
> CFSTR_FILEDESCRIPTOR/CFSTR_FILECONTENTS formats (corresponding to the mime type
> application/x-moz-file-promise) which I believe Paint supports just fine (don't

this windows format is not accepted by apps like mspaint etc. For trial, drag attachment like a txt file to an editor or image to mspaint and it fails. Yet dragging to desktop works because windows shell accepts the stream setup by this format.

To briefly repeat a comment in my patches, kfilePromiseMime does not send data as stream with CFSTR_FILECONTENTS contrary to the code. The flavor is assigned cf_hdrop and that doesn't have kfilepromisemime data implemented. 
If however you have the flavor kURLmime included along with it then CFSTR_FILECONTENTS gets set and the stream is called (another non sequitur for sure)

Comment 39

[Phil Lacy]

Attachment: patched to trunk

Comment 40

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #38)
> (In reply to comment #37)
> > CFSTR_FILEDESCRIPTOR/CFSTR_FILECONTENTS formats (corresponding to the mime type
> > application/x-moz-file-promise) which I believe Paint supports just fine (don't
> 
> this windows format is not accepted by apps like mspaint etc. For trial, drag
> attachment like a txt file to an editor or image to mspaint and it fails. Yet
> dragging to desktop works because windows shell accepts the stream setup by
> this format.

Reading the MSDN docs, it sounds like TYMED_FILE is a much better way to go about this.  If I'm reading correctly it passes ownership of the temporary file to the drop target.

"TYMED_FILE

    The storage medium is a disk file identified by a path. If the STGMEDIUM punkForRelease member is NULL, the destination process should use OpenFile to delete the file."

Comment 41

[Phil Lacy]

With my tree of patches I can test comment 25 Rob's delete on exit, or Kyle's comment 40. But this bug is in limbo until other bugs are resolved on nsDataObj (setting depends on).

Comment 42

[Phil Lacy]

(In reply to comment #25)
> I think that setting the delete-on-close flag on the file handle when we make
> it will avoid this issue.

If we create the file with the flag to delete on close, we still have to use a timer or some mechanism to delay closing our handle. My patch deletes the file which is final and doesn't depend on the external app to close it's handle

Comment 45

[Phil Lacy]

Attachment: corrects dragging images to external apps

This may be bitrotted by after checkin of bug CF_HDROP but can be patched to current tree and verified for review. After that checkin it will need an updated patch.
This patch deletes its own temp file in 500ms so the case of crash leaving temps is remote. This patch will allow jpg and other images in content to be dragged to mspaint.exe.  Other apps may require temp files but that is not part of this bug.

Comment 47

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

+  // We are done with this object but external apps may still
+  // need to open the temp file that we need to delete
+  // the timer can own this object now. Delete file anyway if
+  // there's problems and destroy this obj.

Please reword this comment, it's confusing ... some punctuation between the second and third lines would help

Phil, what do you think about comment #40? Would that work?

Comment 48

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #47)
> Phil, what do you think about comment #40? Would that work?

I experimented with it a bit, Paint doesn't support TYMED_FILE either.

Comment 49

[Phil Lacy]

In this bug the drop target is calling the shots, and they don't support all. MSDN seems to have volumes on various format possibilities, but in the end CF_HDROP (files on disk) seems to be the ubiquitous format targets expect finding in the DataObject.

I've been inspired by Kyle to try and get a test up. I think it may be doable using threads (but I'm not as quick as most, hopefully this week)

Comment 50

[Leo Davidson]

Deleting the file on a timer is still wrong. You can't know how long the app the file is dropped on will take to open and/or process the file.

The correct thing to do is to delete the file when the data object's ref-count reaches zero. The temp-file is part of the data object and should have the same lifetime. (Obviously if Firefox exits before the data object is released then that's life and you could attempt to delete it then as a last resort.)

Comment 51

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #50)
> Deleting the file on a timer is still wrong. You can't know how long the app
> the file is dropped on will take to open and/or process the file.
> 
> The correct thing to do is to delete the file when the data object's ref-count
> reaches zero. The temp-file is part of the data object and should have the same
> lifetime. (Obviously if Firefox exits before the data object is released then
> that's life and you could attempt to delete it then as a last resort.)

This is what's done now.  The problem that Phil is trying to solve is that Paint (and other apps) release the IDataObject before they load the file because they expect (rightfully so) that the file name given will continue to exist after they do so.  CF_HDROP doesn't contemplate temporary files, CF_FILECONTENTS/etc are what do but Paint doesn't support those formats.

FWIW, you can't drag images from IE to Paint either.

Comment 52

[Phil Lacy]

(In reply to comment #51)
> (In reply to comment #50)
> > Deleting the file on a timer is still wrong. You can't know how long the app
> > the file is dropped on will take to open and/or process the file.
> > 

Mspaint and probably most apps open the file after releasing the drop. I'm not in favor of 10 sec, I know 500 ms is enough for mspaint and I'd say 100ms is even reasonable for catching most needs.

I don't know about photoshop. I'll try a few other apps and report back.

> 
> FWIW, you can't drag images from IE to Paint either.

I'm thinking drag and drop of temp files will have other uses, for xul apps maybe. I know thunderbird would be able to drag attachments to other apps in that manner.  We probably ought to have a documented flavor/transferdata for temp files.

Comment 53

[Lee Binder]

Why not simply rewind and reimplement the (known-to-work!) code from FF 3.x back into FF 3.5x? Why has something been changed which has always worked? Just for the GENERAL SAKE of CHANGING, because it makes those quixotic pencil-pusher coders, for which automatically altering = improving, feel they actually achieve something (without properly thinking about consequences, if at all) ???

If the altered way of handling drag&drop of grfx into apps makes sense for certain specific apps then there needs to be a handler WITHIN FF which needs to detect if the grfx is actually dragged into one such an app.

If such a query cannot be coded because of the order of events during drag&drop, then it is REALLY TIME TO REWIND BACK TO THE OLD METHOD.

My 2 cc. Sorry if I am making too much sense..

Comment 54

[Phil Lacy]

(In reply to comment #53)
> Why not simply rewind and reimplement the (known-to-work!) code from FF 3.x
> back into FF 3.5x? Why has something been changed which has always worked? 

This duped a lot of old bugs so I'm not sure of the window this was working

Comment 55

[Lee Binder]

you mean there is more disadvantages of the old handling looking at how drag&drop of grfx into win apps is handled in ALL current versions of Windows (XP, Vista, 7)? So it is the percentage of users running a particular version of Windows which would have justified implementing the new behavior in FF 3.5x?

Comment 56

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #53)
> Why not simply rewind and reimplement the (known-to-work!) code from FF 3.x
> back into FF 3.5x? Why has something been changed which has always worked? Just
> for the GENERAL SAKE of CHANGING, because it makes those quixotic pencil-pusher
> coders, for which automatically altering = improving, feel they actually
> achieve something (without properly thinking about consequences, if at all) ???

The code was changed so that images could be dragged and dropped from Firefox to Photoshop.

> If the altered way of handling drag&drop of grfx into apps makes sense for
> certain specific apps then there needs to be a handler WITHIN FF which needs to
> detect if the grfx is actually dragged into one such an app.

That's not possible on Windows

> If such a query cannot be coded because of the order of events during
> drag&drop, then it is REALLY TIME TO REWIND BACK TO THE OLD METHOD.
> 
> My 2 cc. Sorry if I am making too much sense..

Please correct me if I'm wrong, but my understanding is that drag and drop to Paint never worked.  All that's changed is that instead of nothing happening an error message appears.  I wouldn't consider that a significant loss of functionality.

Is there an application where the drag and drop used to succeed in 3.0.x and now fails?

Comment 57

[Lee Binder]

(In reply to comment #56)
 
> Is there an application where the drag and drop used to succeed in 3.0.x and
> now fails?

dragging into Total Commander now produces a (much larger!) bmp. This has been discussed in a parallel bug report here which, however, has been marked as duplicate of this bug (which is correct I would say because it is the exact same underlying problem).

In FF 3.0x I would copy the img address into clipboard via right click, and in PS Ctrl. or Apple O, Ctrl. or Apple V, ENTER .. ;)

Comment 58

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #57)
> (In reply to comment #56)
> 
> > Is there an application where the drag and drop used to succeed in 3.0.x and
> > now fails?
> 
> dragging into Total Commander now produces a (much larger!) bmp. This has been
> discussed in a parallel bug report here which, however, has been marked as
> duplicate of this bug (which is correct I would say because it is the exact
> same underlying problem).

Hmm, this sounds like a separate problem, namely that we are advertising drop flavors in the wrong order (the CF_HDROP should come absolutely last since it's a dirty hack.).

Comment 59

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #56)
> > 
> > > Is there an application where the drag and drop used to succeed in 3.0.x and
> > > now fails?
> > 
> > dragging into Total Commander now produces a (much larger!) bmp. This has been
> > discussed in a parallel bug report here which, however, has been marked as
> > duplicate of this bug (which is correct I would say because it is the exact
> > same underlying problem).
> 
> Hmm, this sounds like a separate problem, namely that we are advertising drop
> flavors in the wrong order (the CF_HDROP should come absolutely last since it's
> a dirty hack.).

Though I checked and we are advertising them in the right order ...

Comment 60

[Phil Lacy]

50 ms doesn't cut it. 100 ms works with mspaint, and MS Photo Editor. Irwinview needs the 100 also. It would be nice to see what photoshop needs as that seems to be pretty universal although I don't have it.

The wordpad.exe bug isn't fixed.  It must be a different problem

Comment 61

[Phil Lacy]

(In reply to comment #57)
> dragging into Total Commander now produces a (much larger!) bmp. This has been

Patching this bug will still not fix drag and drop of images in their mime format (it will create the bmp format).

Patching bug 484667 will create a temp file from kFilePromiseMime. If it's modified to remove the image-to-bmp code and kNativeImageMime, then images can be dragged in their native format.

Comment 62

[Lee Binder]

very simple and pragmatic suggestion:

- drag&drop: new method (FF 3.5x)
- Ctrl. + drag&drop: old method (FF 3.0x)

following everything there are advantages and disadvantages to both handlers, which means the old handler needs to be re-implemented anyway .. (?)

Comment 63

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

Comment on attachment 414793 [details] [diff] [review]
corrects dragging images to external apps

OK, it sounds to me like this is a reasonable thing to do, assuming we want to support apps that support CF_HDROP only.

Comment 64

[Phil Lacy]

thanks for review.

I'll fix comment 47, the 500 ms stays as is. After the other bugs on dependency tree land this one will have to be unbitrotted then I'll ask for checkin.

Comment 65

[dawe]

(In reply to comment #62)
> very simple and pragmatic suggestion:
> 
> - drag&drop: new method (FF 3.5x)
> - Ctrl. + drag&drop: old method (FF 3.0x)
> 
> following everything there are advantages and disadvantages to both handlers,
> which means the old handler needs to be re-implemented anyway .. (?)

Sounds like a good plan!!

Comment 66

[Phil Lacy]

Attachment: for checkin, forward r+=roc

patch for checkin
r+=roc
un-bitrotted due to dependent bug;
and, with reformatting. A large portion of change was put in an if{} block and had to be reformatted. See approved code for code change.

Comment 67

[Phil Lacy]

Attachment: for checkin, r+=roc fixes possible leak

roc, I notice the original code had a possible leak point I'd like to mark this one for checkin if ok

around line 1492:
    // Save the bitmap to a temporary location.      
    nsCOMPtr<nsIFile> dropFile;
    rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(dropFile));
    if (!dropFile) {
      GlobalFree(bits); <<<<<<<<<<<<<<<<<<< missing 
      return E_FAIL;
    }

Comment 68

[Jim Mathies [:jimm]]

+    fileHdr.bfType		    = ((WORD) ('M' << 8) | 'B');
+    fileHdr.bfSize		    = GlobalSize (bits) + sizeof(fileHdr);
+    fileHdr.bfReserved1 	= 0;
+    fileHdr.bfReserved2 	= 0;
+    fileHdr.bfOffBits		  = (DWORD) (sizeof(fileHdr) + bmpHdr->biSize);

Phil, can we kill those tabs as long as we're in here?

and on the end - 

+    if (NS_FAILED(rv)) { 
+      GlobalFree(bits);
+      return E_FAIL;
+    }
+
+    GlobalFree(bits);
   }

maybe just:

+    GlobalFree(bits);
+
+    if (NS_FAILED(rv))
+      return E_FAIL;
+  }

Comment 69

[Phil Lacy]

Attachment: final nits and fix possible leak

Comment 70

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/580ac8e615d8

Comment 71

[Lee Binder]

dragging to paint now works in the 3.7 alpha, but dragging to Total Commander still produces a bmp, even if dragged with Ctrl or Alt or Shift or any combo of these .. :(

Comment 72

[jpotter]

(In reply to comment #71)
> dragging to paint now works in the 3.7 alpha, but dragging to Total Commander
> still produces a bmp, even if dragged with Ctrl or Alt or Shift or any combo of
> these .. :(

See my comments to Bug 502453 (https://bugzilla.mozilla.org/show_bug.cgi?id=502453#c5) for the reason why this specific problem is happening.

It seems to me there are/have been several subtly different drag & drop issues which have been marked as duplicates by people who have not quite appreciated the subtle differences.

The problem with Total Commander and other apps producing a bmp rather than a jpg/png/etc on drops from FireFox is because the dataobject provides two fundamentally different sets of data, which is clearly against the intention of COM data transfer. An IDataObject can provide the SAME data in many different ways, but it is not designed to provide completely different data.

Comment 73

[Phil Lacy]

Images are currently converted to .bmp in the backend.
It needs to be investigated at what point that code was inserted and the associated bug reviewed to the reason it is so.
http://mxr.mozilla.org/comm-central/source/mozilla/widget/src/windows/nsDataObj.cpp#1388

Comment 74

[Phil Lacy]

also this is the location of the description of our flavors:
http://mxr.mozilla.org/comm-central/source/mozilla/widget/public/nsITransferable.idl#47

The bmp file appears only to come from kNativeImageMime.

Comment 75

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I wonder if we could just dump the contents of the IStream to disk for the CF_HDROP ...

Comment 76

[Phil Lacy]

Bug 440911 is the bug. I didn't look at it in detail. Maybe Jim M. can jog his memory on anything we don't need to revisit. when I was in that file I couldn't understand why it was being converted either unless it was expected all the other flavors would be implemented at some later point?

Comment 77

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I filed Bug 562344 to fix the new issue.

Comment 78

[Lee Binder]

Thank you very much jp, Kyle and Phil for redirecting your's attention to this bug!