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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: philbaseless-firefox, Assigned: philbaseless-firefox)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.1 ?)

Details

Attachments

(2 attachments, 14 obsolete attachments)

96.66 KB, message/rfc822
Details
12.74 KB, patch
jimm
: review+
Details | Diff | Splinter Review
Assignee

Description

10 years ago
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.
Attachment #379796 - Flags: review?(emaijala)
Assignee

Updated

10 years ago
Component: Drag and Drop → Widget: Win32
QA Contact: drag-drop → win32
Assignee

Comment 1

10 years ago
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.
Summary: windows drag and drop could prematurely delete temp file used as source data → windows drag and drop does not use temporary file when needed
Assignee

Comment 2

10 years ago
this eml file should be usable to test dragging the image to mspaint.
Assignee

Comment 3

10 years ago
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.
Attachment #379796 - Attachment is obsolete: true
Attachment #382231 - Flags: review?(emaijala)
Attachment #379796 - Flags: review?(emaijala)
Assignee

Comment 4

10 years ago
ref bug 245861 and bug 203307 for background.

Updated

10 years ago
Attachment #382231 - Flags: review?(emaijala) → review-

Comment 5

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

Comment 6

10 years ago
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.
Attachment #382231 - Attachment is obsolete: true
Attachment #382425 - Flags: review?(emaijala)
Assignee

Comment 7

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

Comment 8

10 years ago
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.
Attachment #382425 - Flags: review?(emaijala)
Assignee

Comment 9

10 years ago
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.
Attachment #382425 - Attachment is obsolete: true
Attachment #382622 - Flags: review?(emaijala)
Assignee

Updated

10 years ago
Blocks: 484667
Assignee

Updated

10 years ago
Attachment #382622 - Flags: review?(emaijala)
Assignee

Comment 10

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

Comment 11

10 years ago
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.
Attachment #382622 - Attachment is obsolete: true
Attachment #383050 - Flags: review?(emaijala)
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?
Attachment #383050 - Flags: review?(emaijala) → review+
Assignee

Comment 13

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

Comment 14

10 years ago
Posted patch added comment (obsolete) — Splinter Review
Need rereview because not adding all previous r+ comments.  See my previous comment.
Attachment #383050 - Attachment is obsolete: true
Attachment #384267 - Flags: review?(emaijala)
Assignee

Comment 15

10 years ago
windows
Flags: blocking1.9.1?
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.
Flags: blocking1.9.1? → wanted1.9.1.x?

Updated

10 years ago
Attachment #384267 - Flags: review?(emaijala) → review+
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.
Assignee

Comment 18

10 years ago
Attachment #384267 - Attachment is obsolete: true
Assignee

Comment 19

10 years ago
Attachment #388391 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Keywords: checkin-needed
Assignee

Updated

10 years ago
Attachment #388398 - Flags: superreview?(roc)
Assignee

Updated

10 years ago
Keywords: checkin-needed
Does this mean that these temporary files are only deleted when Firefox exits?
Assignee

Comment 21

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

Comment 23

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

Comment 27

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

Updated

10 years ago
Status: NEW → ASSIGNED
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Assignee

Updated

10 years ago
Attachment #388398 - Flags: superreview?(roc)
Assignee

Comment 28

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

Comment 29

10 years ago
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.
Attachment #388398 - Attachment is obsolete: true
Attachment #399024 - Flags: review?(roc)
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.
Assignee

Comment 31

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

Comment 34

10 years ago
Posted patch nsITimer in with nsDataObj (obsolete) — Splinter Review
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.
Attachment #399024 - Attachment is obsolete: true
Attachment #399404 - Flags: review?(emaijala)
Attachment #399024 - Flags: review?(roc)
Assignee

Comment 35

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

Comment 36

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

Comment 38

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

Updated

10 years ago
Depends on: 491239
Assignee

Comment 39

10 years ago
Posted patch patched to trunk (obsolete) — Splinter Review
Attachment #399404 - Attachment is obsolete: true
Attachment #399404 - Flags: review?(emaijala)
(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."
Assignee

Comment 41

10 years ago
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).
Depends on: CF_HDROP, 528731
Assignee

Updated

10 years ago
No longer depends on: 528731
Assignee

Comment 42

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

Updated

10 years ago
Duplicate of this bug: 415632
Assignee

Updated

10 years ago
Duplicate of this bug: 213170
Assignee

Comment 45

10 years ago
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.
Attachment #412357 - Attachment is obsolete: true
Attachment #414793 - Flags: review?(roc)
Assignee

Updated

10 years ago
Summary: windows drag and drop does not use temporary file when needed → windows drag and drop does not use temporary file when needed--fixes dragging image to apps that can handle images
Assignee

Updated

10 years ago
Duplicate of this bug: 502453
Assignee

Updated

10 years ago
No longer depends on: 491239
+  // 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?
(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.
Assignee

Comment 49

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

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

Comment 52

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

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

Comment 54

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

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

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

Comment 60

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

Comment 61

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

10 years ago
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 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.
Attachment #414793 - Flags: review?(roc) → review+
Assignee

Comment 64

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

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

Comment 66

10 years ago
Posted patch for checkin, forward r+=roc (obsolete) — Splinter Review
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.
Attachment #414793 - Attachment is obsolete: true
Attachment #417866 - Flags: review+
Assignee

Updated

10 years ago
Keywords: checkin-needed
Assignee

Comment 67

10 years ago
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;
    }
+    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;
+  }
Assignee

Updated

10 years ago
Attachment #418317 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Attachment #418317 - Attachment is obsolete: false
Assignee

Updated

10 years ago
Attachment #418317 - Flags: review+
Assignee

Updated

10 years ago
Attachment #417866 - Attachment is obsolete: true
Attachment #417866 - Flags: review+
Assignee

Comment 69

10 years ago
Attachment #418317 - Attachment is obsolete: true
Attachment #418323 - Flags: review?(jmathies)

Updated

10 years ago
Attachment #418323 - Flags: review?(jmathies) → review+
http://hg.mozilla.org/mozilla-central/rev/580ac8e615d8
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 71

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

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

Comment 73

9 years ago
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
Assignee

Comment 74

9 years ago
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.
I wonder if we could just dump the contents of the IStream to disk for the CF_HDROP ...
Assignee

Comment 76

9 years ago
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 78

9 years ago
Thank you very much jp, Kyle and Phil for redirecting your's attention to this bug!
You need to log in before you can comment on or make changes to this bug.