User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3 See Bug 224209 When you "Save As..." an image from a php file (i.e. "attachment.php&postid=163252") it shows you the real filename (i.e. "mypicture.gif"). That's good. But when you're dragging the image to desktop you get the filename "attachment.gif". Reproducible: Always Steps to Reproduce:

Marking dependent on bug 224209 for now, since the arch work there may make this easier...

Not just PHP, of course, but any sort of "dynamic image-serving scripts" (Bugzilla attachment CGIs)....

This behaviour is still occuring. Would a bugfix for this need intricate knowledge of firefox internals or is this something a person with a couple years of general C++ experience could fix in a few days?

I would have thought someone with that experience would be able to answer the question by having a quick dig into the source. My guess is that it would be fairly simple by comparing what Save As does to get the correct filename.

A good testcase can be found here: http://forum.magicball.net/attachment.php?s=&postid=163252 It has: > Content-disposition: inline; filename*=utf-8''cfdesktop.jpg So it is named "cfdesktop.jpg" when content disposition is used, attachment.php otherwise.

Bug 440930 and bug 449588 are dupes of this bug, as I wrote there. Would somebody mark them as dupes, please?

As I said in Comment #4 of bug 449588, it isn't a dupe because solving this bug is only one of the possibilities to fix bug 449588. The other being *not* to use content-disposition in File > Save as. This other solution might be easier to implement and the inconsistency could be fixed faster. It will always be possible to revert once this bug is solved. Please feel free to reopen it if you agree.

We're not going to stop using content-disposition in File > Save as.

Comment on attachment 414866 [details] [diff] [review] Draft Patch Patch itself is fairly straightforward, though the code is mostly duped from helper functions that go along with nsExternalAppHelperService. It would be cool if we could somehow expose those functions more widely, maybe in nsNetUtil? I also can't see an easy way to test this.

Any chance for some mochitests?

Comment on attachment 414866 [details] [diff] [review] Draft Patch >diff --git a/content/base/src/nsContentAreaDragDrop.cpp b/content/base/src/nsContentAreaDragDrop.cpp >--- a/content/base/src/nsContentAreaDragDrop.cpp >+++ b/content/base/src/nsContentAreaDragDrop.cpp >@@ -86,17 +86,19 @@ > #include "nsIPresShell.h" > #include "nsPresContext.h" > #include "nsIDocShellTreeItem.h" > #include "nsIFrame.h" > #include "nsRange.h" > #include "nsIWebBrowserPersist.h" > #include "nsEscape.h" > #include "nsContentUtils.h" >+#include "nsIMIMEHeaderParam.h" > #include "nsIMIMEService.h" >+#include "imgICache.h" > #include "imgIContainer.h" > #include "imgIRequest.h" > #include "nsContentCID.h" > #include "nsDOMDataTransfer.h" > #include "nsISelectionController.h" > #include "nsFrameSelection.h" > #include "nsIDOMEventTarget.h" > #include "nsWidgetsCID.h" >@@ -816,16 +818,17 @@ nsTransferableFactory::GetNodeString(nsI > > > nsresult > nsTransferableFactory::Produce(nsDOMDataTransfer* aDataTransfer, > PRBool* aCanDrag, > PRBool* aDragSelection, > nsIContent** aDragNode) > { >+ nsresult rv; > NS_PRECONDITION(aCanDrag && aDragSelection && aDataTransfer && aDragNode, > "null pointer passed to Produce"); > NS_ASSERTION(mWindow, "window not set"); > NS_ASSERTION(mSelectionTargetNode, "selection target node should have been set"); > > *aDragNode = nsnull; > > nsIContent* dragNode = nsnull; >@@ -974,22 +977,66 @@ nsTransferableFactory::Produce(nsDOMData > > // grab the image data, and its request. > nsCOMPtr<imgIContainer> img = > nsContentUtils::GetImageFromContent(image, > getter_AddRefs(imgRequest)); > > nsCOMPtr<nsIMIMEService> mimeService = > do_GetService("@mozilla.org/mime;1"); Do you actually need this if content-disposition is used? >+ if (NS_SUCCEEDED(rv) && !mImageDestFileName.IsEmpty()) { >+ mImageDestFileName.ReplaceChar(FILE_PATH_SEPARATOR >+ FILE_ILLEGAL_CHARACTERS, '-'); >+ mImage = img; >+ imgUri = nsnull; >+ } >+ } >+ } >+ } >+ // If we can't use the cache to get the correct filename, at least >+ // fix up the file's extension >+ if (imgUri && mimeService) { Setting imgURI to nsnull is a bit ugly, IMO. Could you just have some PRBool variable to indicate if file's extension needs to be fixed up?

Comment on attachment 414866 [details] [diff] [review] Draft Patch So if you could update the patch. I'll re-review

Comment on attachment 414866 [details] [diff] [review] Draft Patch Thanks, it'll be a few days before I have time to update the patch. Any thoughts on avoiding the code duplication? (Most of it is copy pasted from nsExternalAppHelperService.cpp where all of the content-disposition helper functions live)

Yeah, it would be good to use the same code as what nsExternalAppService uses. That one seems to handle also multipart channels. Not sure yet which interface should have the new method.

nsIMIMEService seems to have similar methods and nsExternalAppService implements that interface anyway.

After thinking about this some more, I think the right way to do this is to fix Bug 356086 so that the underlying channel will have a content-disposition-filename that we can pull straight out of imgICache.

This bug (first identified ~10 years ago(!)) is still occurring. The old test case posted earlier still shows the problem: http://forum.magicball.net/attachment.php?s=&postid=163252.