Closed Bug 263697 Opened 20 years ago Closed 20 years ago

[FIXr]Content-Disposition headers no longer looked at when saving documents

Categories

(Core Graveyard :: File Handling, defect, P2)

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8alpha5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file)

With the checkin for bug 160454, we no longer look at the content-disposition header when saving files via "save page as". We should fix that. We have a document object, so we should be able to get the content-disposition header off of it. jst, I know we've talked about this in the abstract before, but we sort of need it in the concrete now. Do you have a preferred way to expose metadata on documents, short of adding more members to nsIDOMNSDocument? I was thinking of adding a getMetaData() method there which would take a string naming the metadata (and for now the only metadata recognized would be "content-disposition"). Or is that too free-form?
(In reply to comment #0) > With the checkin for bug 160454, we no longer look at the content-disposition > header when saving files via "save page as". We should fix that. We have a > document object, so we should be able to get the content-disposition header off > of it. Isn't it a bigger problem when we initiate a "save link target as"? In that case we don't have a document object. And if we /do/ already have a document object, then the user can clearly see (and be aware of) what they are saving.
> In that case we don't have a document object. True. So not much we can do. > then the user can clearly see (and be aware of) what they are saving. That's not the point. The point is that the Content-Disposition header can say what the filename should be when the document is saved. We used to respect that. Now we don't. This _will_ break some pages, so we need to fix it.
(In reply to comment #2) > That's not the point. The point is that the Content-Disposition header can say > what the filename should be when the document is saved. We used to respect > that. Now we don't. This _will_ break some pages, so we need to fix it. Ah yes. rfc2183 states "If the receiving MUA writes the entity to a file, the suggested filename should be used as a basis for the actual filename, where possible... important that the receiving MUA not blindly use the suggested filename. The suggested filename SHOULD be checked..." etc etc. That was referring to a MUA, but that also applies to a browser I figure (or maybe there's a more up-to-date rfc). Anyhooo... can the content-disposition header be examined in WBP, or has it all been squirelled away by then (or even discarded)?
wbp is too late, you want it before the file picker is shown
(In reply to comment #4) > wbp is too late, you want it before the file picker is shown ah, true... Didn't someone say recently that some of this stuff is discarded when the page is loaded (i.e it isn't even stored with the file in the cache)? So is content-disposition one of those things? (I can't see the stuff we need in the cache either in 1.7.3 or 1.8a4). anyway, my train of thought was following the scenario of saving link targets, but that is all bogus because we have to pick a filename (and show the filepicker) before we even start downloading the file :)
the cache does store this (it stores all http headers), but the page may not be in the cache... I think the document object (or somesuch) should get this header from the channel and expose it at least to chrome scripts. or maybe it should just expose the filename.
Note that I'm just waiting on jst's comments on comment 0. Once we have agreement on that, I'll expose the content-disposition header on the document.
Adding a getMetaData() method to our document objects scares me a bit due to the almost inevitable conflict with global function names already used on some random webpage out there... But I do want to see this functionality exposed... Do we want to risk sites breaking? Or do we want to push this into nsIDOMWindowUtils for now?
We could do that, I guess... There are some cases of documents without windows, though (eg if we ever implement "save as, complete" for "save link"). Perhaps we could call it getGeckoMetaData() or something? To make the risk of collisions very unlikely?
What we really need is for the non-existent W3C DOM WG to specify how HTTP headers etc are exposed on a document and everyone plays happily together. Well, that ain't going to happen any time soon, so maybe we just need to bite the bullet, or involve WHATWG?
I'd say bite the bullet ('cause we need to fix this bug for 1.8a5) _and_ involve WHATWG.
why not create a new, chrome-only interface implemented by our documents? is there really need to expose this information via the DOM to unprivileged script?
> why not create a new, chrome-only interface implemented by our documents? Last I checked, there was no way to keep non-chrome script from getting at such an interface once the document had been QIed to it by chrome (thanks to dynamic interface flattening and so forth).
OK, this is making bugzilla a pain to use. We need some consensus here. jst, if you prefer that we use nsIDOMWindowUtils I can do that (with nsIDocument changes to let the window get the data, I guess).
Severity: normal → major
Keywords: regression
I just figured that on a photo gallery like in http://inconstruction.kairo.at/?d=g&i=51&m=v (and I have lots of those), when you "Save image as...", you get a .htm filename suggested for saving though Mozilla already knows the file is an image and decoded it that way, the image has a correct image/* MIME type in content type set and has a proposed file name in content-disposition. That all (at least MIME type and file name) was worth something until HEAD requests got removed, now we seems to ignore all of that and users end up saving a file they can't open (as file managers may link applications by extension by default). This is clearly a big regression and we shouldn't even ship a beta with that one, so we should dig up a solution quite soon. I know you're thinking about that issue, I just wanted to give you a real life case where it has really bad impact, as we even mess up the file type completely.
Robert, that case is not actually covered by this bug. At the moment we really don't have a way to fix that case at all; if you can suggest one, I'd love to hear it. Please file a separate bug on it.
a suggestion would be to use mimeservice.getPrimaryExtension(document.contentType, document.location.fileExtension) (pseudocode) let me note that the content-disposition part kairo mentions _is_ covered by this bug
biesi, kairo's talking about "save image as", not saving the current document... For save image as we don't have access to the header. For the document we can have access to it, and should. I'm really not sure what getPrimaryExtension has to do with that...
(In reply to comment #18) > biesi, kairo's talking about "save image as", not saving the current document... > For save image as we don't have access to the header. For the document we can > have access to it, and should. Unfortunately when the only thing in the browser window is an image (i.e not part of an html doc), 'save image as' and 'save page as' both do the same thing.
(In reply to comment #18) > biesi, kairo's talking about "save image as", not saving the current document... > For save image as we don't have access to the header. For the document we can > have access to it, and should. > > I'm really not sure what getPrimaryExtension has to do with that... er, yeah. getPrimaryExtension would only be useful to get an extension if we don't have one from other sources...
OK, as it turns out to be a different issue then, I filed bug 264757 for comment #15 as another regression caused by the removal of HEAD.
(In reply to comment #14) > OK, this is making bugzilla a pain to use. We need some consensus here. jst, > if you prefer that we use nsIDOMWindowUtils I can do that (with nsIDocument > changes to let the window get the data, I guess). Sorry for not replying sooner. I'd recomment putting this in nsIDOMWindowUtils for now, and if at some point this needs to migrate to a more standard place then so be it, but for now let's keep this private...
Attached patch Patch (deleted) — Splinter Review
Comment on attachment 165929 [details] [diff] [review] Patch biesi, jst, would you review?
Attachment #165929 - Flags: superreview?(jst)
Attachment #165929 - Flags: review?(cbiesinger)
Comment on attachment 165929 [details] [diff] [review] Patch xpfe/communicator/resources/content/contentAreaUtils.js could pass {} as the out param, in both calls. then you wouldn't need dummy. you could use "return mhp.getParameter(..);" and avoid the local var that way, you can even avoid one of the try..catch blocks, I think r=biesi either way.
Attachment #165929 - Flags: review?(cbiesinger) → review+
> you could use "return mhp.getParameter(..);" No, I couldn't. It can return an empty string...
Comment on attachment 165929 [details] [diff] [review] Patch sr=jst
Attachment #165929 - Flags: superreview?(jst) → superreview+
I think we should take this regression fix for 1.8a5. It's pretty noticeable...
Flags: blocking1.8a5?
Assignee: Time_lord → bzbarsky
Priority: -- → P2
Summary: Content-Disposition headers no longer looked at when saving documents → [FIXr]Content-Disposition headers no longer looked at when saving documents
Target Milestone: --- → mozilla1.8alpha5
Comment on attachment 165929 [details] [diff] [review] Patch a=asa for 1.8a5 checkin.
Attachment #165929 - Flags: approval1.8a5+
Flags: blocking1.8a5? → blocking1.8a5+
Fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
No longer blocks: 271981
Depends on: 271981
Is it possible to port this for aviary branch? It would help UMO bug 275900
How would porting this to aviary branch help exactly? This bug was a fix to a Seamonkey-only regression caused by a Seamonkey-only checkin. Bug 275900 comment 8 is flat out wrong.
Blocks: 294759
While Save Page As works fine, Save Link Target As is still broken with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050701. Firefox trunk is now affected as well since bug 294759 synced contextAreaUtils.js. See bug 299372.
This bug is about "save page as", so comment 33 is pretty unrelated.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: