Closed Bug 224209 Opened 16 years ago Closed 5 years ago

wrong filename on title when viewing an image from a PHP file (Content-Disposition is not used)

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: idobeeri, Assigned: akshendra521994, Mentored)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [lang=c++])

Attachments

(3 files, 4 obsolete files)

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:
Attached image explanation of the bug
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.
Product: Browser → Seamonkey
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)
Severity: trivial → enhancement
Status: UNCONFIRMED → NEW
Component: General → Networking
Ever confirmed: true
OS: Windows XP → All
Product: Mozilla Application Suite → Core
Hardware: PC → All
Summary: wrong filename on title when viewing an image from a PHP file → wrong filename on title when viewing an image from a PHP file (Content-Disposition is not used)
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. ***
Duplicate of this bug: 341489
Assignee: general → nobody
QA Contact: general → networking
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.
Blocks: 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.
Mentor: bzbarsky
Component: Networking → DOM
Whiteboard: [lang=c++]
Attached patch 224209.patch (obsolete) — Splinter Review
Its not working yet. But I would like to know am I on the right path?
Attachment #8511602 - Flags: feedback?(bzbarsky)
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.
Attachment #8511602 - Flags: feedback?(bzbarsky) → feedback+
Attached patch 224209.patch (obsolete) — Splinter Review
This one is working.
Attachment #8511602 - Attachment is obsolete: true
Attachment #8513501 - Flags: feedback?(bzbarsky)
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?
Attachment #8513501 - Flags: feedback?(bzbarsky) → feedback+
> >+  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)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
Attached patch 224209.patch (obsolete) — Splinter Review
Please can you assign that to me. Thanks!!
Attachment #8513501 - Attachment is obsolete: true
Attachment #8513707 - Flags: review?(bzbarsky)
Assignee: nobody → akshendra521994
Comment on attachment 8513707 [details] [diff] [review]
224209.patch

r=me

Do you need this pushed to try, or can you do that?
Flags: needinfo?(akshendra521994)
Attachment #8513707 - Flags: review?(bzbarsky) → review+
I don't have a Mozilla hg account. So I cannot push it.
Flags: needinfo?(akshendra521994)
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?
Status: NEW → ASSIGNED
Attached patch 224209.patch (obsolete) — Splinter Review
Added the commit message
Attachment #8513707 - Attachment is obsolete: true
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8513738 - Attachment is obsolete: true
Attachment #8514234 - Flags: review?(bzbarsky)
Comment on attachment 8514234 [details] [diff] [review]
224209_wrongFilenameOnTitleForAttachment.patch

Thanks.  Pushed https://tbpl.mozilla.org/?tree=Try&rev=741e628dafad
Attachment #8514234 - Flags: review?(bzbarsky) → review+
Looks nice and green.
Keywords: checkin-needed
Akshendra Pratap Singh, thank you again for the patch!
Thanks for the help
https://hg.mozilla.org/mozilla-central/rev/1a8f290db545
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Hip, hip, hooray.
Depends on: 1285835
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.