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)
Core Graveyard
File Handling
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8alpha5
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
Biesinger
:
review+
jst
:
superreview+
asa
:
approval1.8a5+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•20 years ago
|
||
> 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)?
Comment 4•20 years ago
|
||
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 :)
Comment 6•20 years ago
|
||
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.
Assignee | ||
Comment 7•20 years ago
|
||
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.
Comment 8•20 years ago
|
||
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?
Assignee | ||
Comment 9•20 years ago
|
||
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?
Comment 10•20 years ago
|
||
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?
Assignee | ||
Comment 11•20 years ago
|
||
I'd say bite the bullet ('cause we need to fix this bug for 1.8a5) _and_ involve
WHATWG.
Comment 12•20 years ago
|
||
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?
Assignee | ||
Comment 13•20 years ago
|
||
> 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).
Assignee | ||
Comment 14•20 years ago
|
||
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
Comment 15•20 years ago
|
||
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.
Assignee | ||
Comment 16•20 years ago
|
||
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.
Comment 17•20 years ago
|
||
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
Assignee | ||
Comment 18•20 years ago
|
||
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...
Comment 19•20 years ago
|
||
(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.
Comment 20•20 years ago
|
||
(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...
Comment 21•20 years ago
|
||
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.
Comment 22•20 years ago
|
||
(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...
Assignee | ||
Comment 23•20 years ago
|
||
Assignee | ||
Comment 24•20 years ago
|
||
Comment on attachment 165929 [details] [diff] [review]
Patch
biesi, jst, would you review?
Attachment #165929 -
Flags: superreview?(jst)
Attachment #165929 -
Flags: review?(cbiesinger)
Comment 25•20 years ago
|
||
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+
Assignee | ||
Comment 26•20 years ago
|
||
> you could use "return mhp.getParameter(..);"
No, I couldn't. It can return an empty string...
Comment 27•20 years ago
|
||
Comment on attachment 165929 [details] [diff] [review]
Patch
sr=jst
Attachment #165929 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 28•20 years ago
|
||
I think we should take this regression fix for 1.8a5. It's pretty noticeable...
Flags: blocking1.8a5?
Assignee | ||
Updated•20 years ago
|
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 29•20 years ago
|
||
Comment on attachment 165929 [details] [diff] [review]
Patch
a=asa for 1.8a5 checkin.
Attachment #165929 -
Flags: approval1.8a5+
Updated•20 years ago
|
Flags: blocking1.8a5? → blocking1.8a5+
Assignee | ||
Comment 30•20 years ago
|
||
Fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•20 years ago
|
Comment 31•20 years ago
|
||
Is it possible to port this for aviary branch? It would help UMO bug 275900
Assignee | ||
Comment 32•20 years ago
|
||
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.
Comment 33•19 years ago
|
||
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.
Assignee | ||
Comment 34•19 years ago
|
||
This bug is about "save page as", so comment 33 is pretty unrelated.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•