User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 I didn't know how to put it so I attached a picture, please take a look. When choosing "View Image", such as "attachment.php&postid=163252" the title shows "attachment.php". But if you choose "Save Image As", you get automatically the real filename in the textbox. Reproducible: Always Steps to Reproduce:

The title just shows the filename from the URL. The filepicker takes into account the content-disposition header..... I don't know whether we care to use content-disposition to set the title in nsMediaDocument::UpdateTitleAndCharset. If we do, we should probably come up with a new Necko interface that channels that can provide a filename hint should share; I'm tired of having to check for both http and multipart channels all over....

bz: i agree with you... we shouldn't have to replicate that code everywhere. it seems like a simple helper function could do the trick... we could add code to nsNetUtils.h, but i suspect it is a fair amount of code. another case where a nsNetUtils helper library would maybe make some sense. or, we could just have all the channels implement yet another interface :-/

and what about some very long parameters on an URL, like: attachment.php&postid=163252asfshxbxbnvn-cv.n-dvnccvndgfmjfmncvnmcxvnxcfbmjfhjhk vmxcbmxcbmxcbmxcbmmbxcbmcbmxcbmsfgjfgkjhfgkghk&user=JohnDoe I don´t want to see crappy strings like this suggested as title.

Hermann, please look at the relevant code before you make comments like that. The query string is in fact stripped off the filename we show in the titlebar.

In addition, if you drag-drop the image on desktop (in opposed to "Save As..."), it also gets the wrong file name.

In addition, if you drag-drop the image on desktop (in opposed to "Save As..."), it also gets the wrong file name.

drag&drop is a different part of the code... can you file a new bug about that?

it should be confirmed at least. I'm changing the component to 'networking' for now (because most of additional code seem to be written in necko)

I want to note: if channels want to expose the filename, the answer these days would not be an additional interface but just another property exposed via nsIPropertyBag2.

Sounds good to me. I think we want to expose both the content-disposition and the content-disposition-filename. The former would be the actual disposition method; the latter would be the filename param. Alternately, we could just expose the header and let all callers parse it as needed... I think we may need to expose the entire header for imagelib (since it promises to return it in some cases).

not all channels may _have_ an entire content-disposition... I can easily imagine a protocol that provides a filename but not a content-disposition header.

OK. So how about we just expose the filename and the disposition type, and change the imagelib api accordingly?

--> Filed Bug 291837 - "Content-Disposition is not used when drag&dropping an image from a PHP file"

*** Bug 339938 has been marked as a duplicate of this bug. ***

Jason, this is the bug where the discussion happened about the sort of API we want for content-disposition in imagelib. You might be interested in this insofar as it affects bug 474536.

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 the title would be "cfdesktop.jpg" when content disposition is used, attachment.php otherwise.

This bug is still present in Firefox 31.0. Eleven years (!) after the original bug report. Please, could someone take care of it?

Channels now expose a filename: See nsIChannel.contentDispositionFilename So what this bug would need is just changes to MediaDocument::GetFileName to consider the channel's filename. Happy to mentor anyone who wants to take this on.

Its not working yet. But I would like to know am I on the right path?

Comment on attachment 8511602 [details] [diff] [review] 224209.patch >+ nsCOMPtr<nsIChannel> channel = do_QueryInterface(imageRequest); imageRequest is not a channel here, so I expect this to return null. Just pass mChannel instead. In fact, I bet you could just examine mChannel inside UpdateTitleAndCharset. Apart from that, and the fact that you're truncating aResult after setting it to the content-disposition filename, this looks resonable.

This one is working.

Image showing working patch

Comment on attachment 8513501 [details] [diff] [review] 224209.patch >+ MediaDocument::UpdateTitleAndCharset(typeStr, mChannel, formatNames, >+ mImageWidth, mImageHeight, status); Please fix the indent. >+ nsCOMPtr<nsIChannel> channel = do_QueryInterface(aChannel); aChannel is already an nsIChannel; why is this QI needed? >+ if (aResult.IsEmpty()) { This should be checked where we do the GetContentDispositionFilename, with a return if not, right?

> >+ nsCOMPtr<nsIChannel> channel = do_QueryInterface(aChannel); > > aChannel is already an nsIChannel; why is this QI needed? > I forgot to qref. I would also like to ask about the charset stuff that is there in the function tail. Because since I return early with the filename(if available) from the channel. The charset is not taken care of. Is it alright? (I got the punctuation correct here :O)

The charset stuff is needed because the GetFilename on the URI returns (possibly-escaped) bytes, not characters. GetContentDispositionFilename handles all the charset bits and returns characters, so doesn't need any extra work here.

Please can you assign that to me. Thanks!!

Comment on attachment 8513707 [details] [diff] [review] 224209.patch r=me Do you need this pushed to try, or can you do that?

I don't have a Mozilla hg account. So I cannot push it.

OK. I just tried doing that, but it looks like you need to rebase across the patches that were checked in for bug 946065 several days ago. Also, add a commit message?

Added the commit message

You still need to rebase to tip, right?

Does that mean I have to repull the repo and do the changes again? Or is there a better way.

You have to pull the updated repo. In terms of updating your changes, there's a sed script at https://gist.github.com/poiru/b266b75473a8d9f71d33 that you can use, or just manually edit the file paths in the patch.... or manually do the changes in the new place, yes.

The files this patch is affection have not been moved yet. I updated the repo and tried a build as well.

Sure they have. What upstream revision is your repo at? Where exactly are you pulling from?

I used "hg pull" and that pulled from "https://hg.mozilla.org/mozilla-central" according to the log.

hg pull doesn't update your local checked out copy. It just pulls the changesets into your local version of the repo. It's the equivalent of "git fetch", not "git pull". You want "hg pull -u" or once you've done the pull "hg up -r default".

Did the changes at new places.

Comment on attachment 8514234 [details] [diff] [review] 224209_wrongFilenameOnTitleForAttachment.patch Thanks. Pushed https://tbpl.mozilla.org/?tree=Try&rev=741e628dafad

Looks nice and green.

Akshendra Pratap Singh, thank you again for the patch!

Thanks for the help

Hip, hip, hooray.