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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Phil]

(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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 3

[Phil]

(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

[Christian :Biesinger (don't email me, ping me on IRC)]

wbp is too late, you want it before the file picker is shown

Comment 5

[Phil]

(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

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

[Johnny Stenback (:jst)]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

[Johnny Stenback (:jst)]

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'd say bite the bullet ('cause we need to fix this bug for 1.8a5) _and_ involve
WHATWG.

Comment 12

[Darin Fisher]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 14

[Boris Zbarsky [:bzbarsky]]

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).

Comment 15

[Robert Kaiser]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

[Phil]

(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

[Christian :Biesinger (don't email me, ping me on IRC)]

(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

[Robert Kaiser]

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

[Johnny Stenback (:jst)]

(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...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165929 [details] [diff] [review]
Patch

biesi, jst, would you review?

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> you could use "return mhp.getParameter(..);"

No, I couldn't.  It can return an empty string...

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 165929 [details] [diff] [review]
Patch

sr=jst

Comment 28

[Boris Zbarsky [:bzbarsky]]

I think we should take this regression fix for 1.8a5.  It's pretty noticeable...

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 165929 [details] [diff] [review]
Patch

a=asa for 1.8a5 checkin.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 31

[alanjstr]

Is it possible to port this for aviary branch?  It would help UMO bug 275900

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

[Steffen Wilberg]

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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

This bug is about "save page as", so comment 33 is pretty unrelated.