It currently lives on nsIMultiPartChannel: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIMultiPartChannel.idl#61 biesi suggests that this would be better off on nsIChannel itself, since several of our channel implementations have a concept of a content disposition. Most specifically, this is required for JAR channels that wrap HTTP channels. If we do this, we get remoting of the content disposition for free, and can drop it from the hashpropertybag.

As advertised. I generally made the contentDisposition setters throw, except where we legitimately need them. (Except in nsBaseChannel.) If you think any of those cases should have it, I can change them. Note that it might be useful on HttpBaseChannel, but we'd need to add an nsHttpResponseHead::SetContentDisposition() method. The relationship between the actual header array and the member strings on nsHttpResponseHead isn't clear to me (they appear to be separate?!), so I erred on the side of not adding it.

Comment on attachment 467893 [details] [diff] [review] patch +++ b/uriloader/base/nsURILoader.cpp + nsCOMPtr<nsIURI> uri; + if (httpChannel) + httpChannel->GetURI(getter_AddRefs(uri)); Don't you want to get this from aChannel even for non-http channels? +++ b/uriloader/exthandler/ExternalHelperAppParent.cpp +ExternalHelperAppParent::GetContentDisposition(nsACString& aContentDisposition) +{ + aContentDisposition.Truncate(); This will definitely need fixing at some point Any particular reason why you didn't make this a readonly attribute? Is this so that extensions can override the value, except that they can't because http doesn't support the setter?

(In reply to comment #2) > +++ b/uriloader/base/nsURILoader.cpp > + nsCOMPtr<nsIURI> uri; > + if (httpChannel) > + httpChannel->GetURI(getter_AddRefs(uri)); > > Don't you want to get this from aChannel even for non-http channels? It did look weird to me, but the old code did exactly this. It might be wrong, but I'm not making it worse :) > +++ b/uriloader/exthandler/ExternalHelperAppParent.cpp > +ExternalHelperAppParent::GetContentDisposition(nsACString& > aContentDisposition) > +{ > + aContentDisposition.Truncate(); > > This will definitely need fixing at some point Crowder's all over it in bug 589025. > Any particular reason why you didn't make this a readonly attribute? Is this so > that extensions can override the value, except that they can't because http > doesn't support the setter? Hmm, good point. I originally made it mutable since nsJARChannel wants to set it, but it can do so internally without the setter. Would you rather it be readonly? (I made some of the setters throw just because I'm not sure if it makes sense for people to mess with them, and if they want to, they can file a bug. But if you think we should make it readonly from the beginning, that's moot!)

(In reply to comment #3) > > Don't you want to get this from aChannel even for non-http channels? > > It did look weird to me, but the old code did exactly this. It might be wrong, > but I'm not making it worse :) OK, I can't figure out why bug 267475 only only added the URI to the one part of the if, but sure, let's leave it as it is for now. > Would you rather it be readonly? (I made some of the setters throw just because > I'm not sure if it makes sense for people to mess with them, and if they want > to, they can file a bug. But if you think we should make it readonly from the > beginning, that's moot!) I just realized that extensions can just keep setting the content-disposition header to override this property, so let's make it readonly.

I'll update the patch today.

Carrying over sr=biesi.

Slight tweak.

Comment on attachment 468874 [details] [diff] [review] patch v2.1 >+NS_IMETHODIMP >+nsIconChannel::GetContentDisposition(nsACString &aContentDisposition) >+{ >+ aContentDisposition.Truncate(); >+ return NS_OK; >+} As discussed on IRC, best to have all non-implementations of GetContentDisposition either return an empty string or throw NS_ERROR_NOT_IMPLEMENTED, rather than a mix. >- nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aRequest)); >- if (httpChannel) { >- httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("content-disposition"), disposition); >- } else { >- nsCOMPtr<nsIMultiPartChannel> multiPartChannel(do_QueryInterface(aRequest)); >- if (multiPartChannel) { >- multiPartChannel->GetContentDisposition(disposition); >- } >+ if (chan) { >+ chan->GetContentDisposition(disposition); > } One-line if doesn't need braces. >+ * Access to the Content-Disposition header if available and if applicable. >+ * This allows getting the preferred handling method, preferred filename, >+ * etc. See RFC 2183. >+ */ >+ readonly attribute ACString contentDisposition; Ah, right. I forgot that the preferred filename is in there, too. I believe we've got lots of code that chops up the the dispo/filename differently, and as a result we've got lots of bugs that seem to hinge on not handling non-standard header formats, or barf when the filename appears at all, or if the filename contains \n or ampersands, or other cases (Bugzilla reports 693 bugs for "content-disposition" before giving up!). I'd love to split the interface here into "contentDisposition" and "contentDispositionFilename", so we can unify the logic for handling splitting the disposition/filename, and get rid of all the different places where we currently parse the raw header. Do you think we can do that for FF4? It'd be nice not to break the interface twice. We could handle just the cases that use the new interface for starters, and then gradually convert all the other uses that use GetHeader. Either way, let's file a followup bug.

(In reply to comment #8) > I'd love to split the interface here into "contentDisposition" and > "contentDispositionFilename" So .contentDisposition would be what it is now, and .contentDispositionFilename would be just the filename bits? Or would you like .contentDisposition to be sans filename? Unless we've got blockers relating to those, let's push it out til 5 -- we're explicitly breaking compat across each major release. Want to file a bug with your thoughts so we can get it on radar for 5?

Comment on attachment 468874 [details] [diff] [review] patch v2.1 Landed with changes: http://hg.mozilla.org/projects/electrolysis/rev/f9a4a332f637

Should you add a comment to this property that callers can use nsIMIMEHeaderParam to parse it?

http://hg.mozilla.org/mozilla-central/rev/f9a4a332f637 (In reply to comment #11) > Should you add a comment to this property that callers can use > nsIMIMEHeaderParam to parse it? Sounds like a good idea, yeah. Jason's working on a patch to split that stuff up and unify the parsing, so I'll let him tweak it.

Looks like we want to back this out (see comment from dev.platform). The IID of nsIChannel was changed between beta4 and beta5, for bugs 589292 and 536324. While I agree that these are virtuous changes, I don't think we should be changing the IIDs of base interfaces so late in the beta cycle. It's going to be very difficult to distinguish between crashes caused by third-party extensions which were compiled against beta4, and crashes caused by issues within our own code. This change *might* be related to the topcrash bugs 591880 and 591678, but I really can't tell. Unless these changes were absolutely necessary for e10s (and it doesn't look to me as if they were), I think these should be backed out and postponed until after branching.

BTW, we should hold off on re-landing this until we change patch to use separate 'contentDispositon' and 'contentDispositionFilename' fields, so we centralize the complicated logic for the various edge cases in parsing.

Bug 589025 is a relatively important bug for e10s, and depends (currently) on these changes. If this is rolled back (or has been rolled back), then we will have to roll those changes back, too. Good news: that bug has a version of a patch that doesn't depend on this one, so we can revert to that, if need-be.

bsmedberg, see previous comment. More work to do but it sounds like we can get what we want.

-> Firefox 5.

We''ll need to make sure to handle "filename*" params, per bug 588781.

Reassigning to nobody. If anyone wants to work on this, feel free!

Similar to previous versions, factors out contentDisposition. Also, adds contentDispositionFilename attribute so people don't have to parse it out on their own. Finally, tests for a couple of the channels (I don't know how to add tests for the other channel types).

Comment on attachment 518829 [details] [diff] [review] patch v3 I have some comments here, but I'm not a peer or owner so any + from me is worth what you pay for it :) It seems in good shape. 1] I love contentdispositionfilename as an interface addition - that will avoid bugs 2] do we have to break nsIMultipartChannel::contentDisposition ? Can't it just silently look it up off the base channel and keep the interface compatible? >+inline nsresult >+NS_GetFilenameFromDisposition(nsAString& aFilename, >+ const nsACString& aDisposition, >+ nsIURI* aURI = nsnull) >+{ >+ aFilename.Truncate(); >+ >+ nsCOMPtr<nsIMIMEHeaderParam> mimehdrpar = >+ do_GetService(NS_MIMEHEADERPARAM_CONTRACTID); >+ if (!mimehdrpar) >+ return NS_ERROR_NOT_AVAILABLE; // XXX ? >+ nsresult rv; nsCOMPtr<nsIMIMEHeaderParam> mimehdrpar = do_GetService(NS_MIMEHEADERPARAM_CONTRACTID, &rv); if (NS_FAILED(rv)) return rv; >+NS_IMETHODIMP >+HttpBaseChannel::GetContentDispositionFilename(nsAString& aContentDispositionFilename) >+{ >+ aContentDispositionFilename.Truncate(); >+ >+ nsresult rv = NS_OK; >+ nsCString disposition; >+ >+ rv = GetContentDisposition(disposition); >+ if (NS_FAILED(rv)) >+ return NS_ERROR_NOT_AVAILABLE; >+ I would pass through the specific exception.. if (NS_FAILED(rv)) return rv;

(In reply to comment #22) > Comment on attachment 518829 [details] [diff] [review] > patch v3 > > 2] do we have to break nsIMultipartChannel::contentDisposition ? Can't it just > silently look it up off the base channel and keep the interface compatible? As I understood it, that was the point - to drop contentDisposition down a layer into nsIChannel instead. If we decide to keep it as part of nsIMultipartChannel, what's the best way to go about that? (Currently, the declaration of contentDisposition from nsIMultipartChannel would conflict with the declaration from nsIChannel - see the implementation of nsPartChannel for an example). Do we drop the "NS_DECL_NSIMULTIPARTCHANNEL" and just add in explicit declarations for the functions we have to implement? Also, would we want to make contentDisposition readonly in nsIMultipartChannel to keep it consistent with nsIChannel? Similarly, would we want to add contentDispositionFilename to nsIMultipartChannel? My gut says "yes" to both of these, but I'm open to arguments either way. The other bits in your comment are easy enough changes, I'll put them in the patch when we have a decision on the nsIMultipartChannel bits.

> Do we drop the "NS_DECL_NSIMULTIPARTCHANNEL" If we keep it on both interfaces, then yes. > Also, would we want to make contentDisposition readonly in nsIMultipartChannel Probably yes. There's no good reason for it to be writable, and I would expect that no one ever writes it. > Similarly, would we want to add contentDispositionFilename to > nsIMultipartChannel Imo, no. It's not being used now, so there's no compat issue. I suspect the only reason to keep this on nsIMultipartChannel is to make it easier to port existing code using nsIMultipartChannel to the new world... but there isn't that much code that uses it. I would just move the API to nsIChannel and not worry about it.

(In reply to comment #24) > I suspect the only reason to keep this on nsIMultipartChannel is to make it > easier to port existing code using nsIMultipartChannel to the new world... but > there isn't that much code that uses it. I would just move the API to > nsIChannel and not worry about it. if boris is fine with it, I am too - he has a much better feel for what interfaces are actually being used. My inclination is to not break apis whenever it can be avoided (ABI I think is somewhat less impt), so I just wanted to raise the question.

Almost the same as the old patch, with changes to return rv instead of NS_ERROR_NOT_AVAILABLE where appropriate.

Comment on attachment 519483 [details] [diff] [review] patch v3.1 I'm not sure the underlying channel's content-disposition filename makes sense for jar channel. I'm also not sure what modules/libjar/test/unit/test_bug589292.js is testing exactly... What's it trying to do? For the idl changes, should probably document that the functions throw NS_ERROR_NOT_AVAILABLE if there is no Content-Disposition? But that's not what jar: does with this patch. And it's not what various nsBaseChannel things do... On the other hand, do any of the nsBaseChannel impls even set mContentDisposition/mContentDispositionFilename? Or are those two members unused? Perhaps better is to just return the empty string if there is no Content-Disposition? Other than those bits, looks good. Do we also want to add a function for extracting the actual disposition type? Or is the uriloader codepath the only place that has to do that?

(In reply to comment #27) > Comment on attachment 519483 [details] [diff] [review] > patch v3.1 > > I'm not sure the underlying channel's content-disposition filename makes sense > for jar channel. That may be the case. Is there something you can think of that would make more sense here? > I'm also not sure what modules/libjar/test/unit/test_bug589292.js is testing > exactly... What's it trying to do? Really just make sure the jar channel doesn't blow up when someone asks for content-disposition or the associated filename. > For the idl changes, should probably document that the functions throw > NS_ERROR_NOT_AVAILABLE if there is no Content-Disposition? But that's not what > jar: does with this patch. And it's not what various nsBaseChannel things > do... On the other hand, do any of the nsBaseChannel impls even set > mContentDisposition/mContentDispositionFilename? Or are those two members > unused? I can certainly make jar return not available if there isn't anything (pending discussion of whether to return not available or just an empty string). As for the nsBaseChannel impls... no, none of them ever set mContentDisposition*, so those can just be whacked and I can make nsBaseChannel do whatever is decided re:not available vs. empty string. > Perhaps better is to just return the empty string if there is no > Content-Disposition? I discussed this with jst before working on this bug. He seemed to prefer not available over empty string, but wanted consistency (which I obviously failed at anyway) over any one solution. If there are strong feelings either way, I'll go with that. Otherwise, I'm tempted to still lean towards not available. > Other than those bits, looks good. Do we also want to add a function for > extracting the actual disposition type? Or is the uriloader codepath the only > place that has to do that? Are you talking about just getting the disposition type, or checking if it's inline or not? Getting the type is a one-liner, so it doesn't buy us a whole lot to factor it out. Checking inline vs. attachment/unknown does happen a few places, so we could factor that out (there are already comments in the code to that effect... looks like they're from you, even).

> Is there something you can think of that would make more sense here? Maybe just handing out an empty filename? Or throwing not available. I'm fine with "not available", but then we need to make sure that HTTP and jar do that as needed too, as well as multipart channels. > Are you talking about just getting the disposition type, or checking if it's > inline or not? The latter is the useful thing. And yes, those are my comments. ;)

New version of the patch. Factors out checking if content-disposition is inline or not, and throws not available in all the appropriate cases (empty content-disposition header, missing filename, and always for filename in jar). This behavior has been documented in the idl. Requesting r from Honza at Brian's request/suggestion

Comment on attachment 520230 [details] [diff] [review] patch v3.2 The documentation for NS_ContentDispositionIsInline should make it clearer what the expected input is (the "disposition" part of a content-disposition header; i.e the part before the parameters start). Also, this pattern: if (x) return PR_FALSE; return PR_TRUE; can just be replaced with: return !x; In this case: return dispToken.IsEmpty() || dispToken.LowerCaseEqualsLiteral("inline") || .... With those two nits, sr=me. This looks great!

OK, I started reviewing this, I'll give you comments soon.

Comment on attachment 520230 [details] [diff] [review] patch v3.2 >+++ b/browser/components/feeds/src/nsFeedSniffer.cpp >@@ -153,27 +155,17 @@ HasAttachmentDisposition(nsIHttpChannel* httpChannel) > // RFC 2183, section 2.8 says that an unknown disposition > // value should be treated as "attachment" ... >+ if (NS_FAILED(rv) || !NS_ContentDispositionIsInline(dispToken)) ... > // We have a content-disposition of "attachment" or unknown Those two comments (or some modification) should be moved to NS_ContentDispositionIsInline. Also on other places this method is called. >+++ b/modules/libjar/test/unit/test_bug589292.js >@@ -0,0 +1,21 @@ >+ try { >+ val = channel.contentDisposition; >+ do_throw("The channel has content disposition?!"); >+ } catch (e) { >+ // This is what we want to happen - there's no underlying channel, so no >+ // content-disposition header is available >+ } You always have to do at least one check, so please change the test to: try { val = channel.contentDisposition; do_check(false, "..."); } catch (e) { do_check(true, "..."); } >+++ b/netwerk/base/public/nsNetUtil.h >+/** Determines whether a content-disposition header shows inline or attachment >+ * @param dispToken type of a Content-Disposition header >+ */ >+inline PRBool >+NS_ContentDispositionIsInline(const nsAString& dispToken) >+{ >+ if (!dispToken.IsEmpty() && >+ !dispToken.LowerCaseEqualsLiteral("inline") && >+ // Broken sites just send >+ // Content-Disposition: filename="file" >+ // without a disposition token... screen those out. >+ !StringHead(dispToken, 8).LowerCaseEqualsLiteral("filename") && >+ // Also in use is Content-Disposition: name="file" >+ !StringHead(dispToken, 4).LowerCaseEqualsLiteral("name")) >+ return PR_FALSE; >+ >+ return PR_TRUE; This is really more readable as: NS_ContentDispositionIsInline(const nsAString& dispToken) { // RFC 2183, section 2.8 says that an unknown disposition // value should be treated as "attachment" return dispToken.IsEmpty() || dispToken.LowerCaseEqualsLiteral("inline") || // Broken sites just send // Content-Disposition: filename="file" // without a disposition token... screen those out. StringHead(dispToken, 8).LowerCaseEqualsLiteral("filename") || // Also in use is Content-Disposition: name="file" StringHead(dispToken, 4).LowerCaseEqualsLiteral("name"); } >+inline nsresult >+NS_GetFilenameFromDisposition(nsAString& aFilename, >+ const nsACString& aDisposition, >+ nsIURI* aURI = nsnull) >+{ >+ rv = mimehdrpar->GetParameter(aDisposition, "filename", >+ fallbackCharset, PR_TRUE, nsnull, >+ aFilename); >+ if (NS_FAILED(rv) || aFilename.IsEmpty()) >+ // Try 'name' parameter, instead. >+ rv = mimehdrpar->GetParameter(aDisposition, "name", fallbackCharset, >+ PR_TRUE, nsnull, aFilename); >+ >+ if (NS_FAILED(rv)) >+ return rv; Just to be safe, Truncate() aFilename again on failure of the parser. >+++ b/uriloader/base/nsURILoader.cpp >@@ -381,64 +381,40 @@ nsresult nsDocumentOpenInfo::DispatchContent(nsIRequest *request, nsISupports * >+ nsCAutoString disposition; >+ rv = aChannel->GetContentDisposition(disposition); > > LOG((" Disposition header: '%s'", disposition.get())); > > if (NS_SUCCEEDED(rv) && !disposition.IsEmpty()) After calls to GetContentDisposition (and -Filename) you probably don't need the check for !disposition.IsEmpty() as it is on success of that method non-empty by definition. Otherwise good job. r=honzab with those details.

carrying forward r=honzab, sr=bz

Comment on attachment 528884 [details] [diff] [review] patch v3.3 I know I'm very late to the party here, but I've got some questions/suggestions. Most of them are API issues, so bz is probably the best arbiter. 1) It seems preferable to me to have a separate 'nsIContentDispositionChannel' interface, and have the channels that actually provide the info (JSChannel, View-source, Multipart, HTTP) implement it. That way we don't need to provide lots of NOT_AVAILABLE stub implementations for all the channel types that don't implement this: WebSockets, JAR, Icon, everything that inherits from nsBaseChannel (FTP, File, ...?), wyciwyg (shouldn't wyciwyg provide the original HTTP channel's C-D?), ExternalProtocolHandler. 2) The interface here isn't as simple as I imagined. My goal was to keep the actual guts of the Content-Disposition header out of the hands of all non-channel code if possible. Most clients of the API ultimately only want to know 2 things; 1) if the content-disposition is a) absent; b) inline; or c) attachment. And 2) what the filename is if any. #1 still seems way too complicated to me: with the new API, to figure out if a channel needs external handling (i.e. C-D is "attachment" or equivalent), we need to call getContentDisposition(), get a MIMEParser, get the channel charset, call GetParam to get the diposition token, and then finally call NS_ContentDispositionIsInline(token). As it is now nsFeedSniffer::HasAttachmentDisposition() and nsDocumentOpenInfo::DispatchContent are basically duplicating a lot of code to figure this out. At a minumum, it would be nice to have an NS_GetContentDispositionType() function to store the common code, but even better would be for the 'contentDisposition' field to simply be an enum with two or three possible values (/INLINE/ATTACHMENT, and maybe NOT_PROVIDED if needed). One problem with that plan, though, is that we've also got code that's doing mysterious (at least to me:) things with the whole raw C-D header: nsDocument.cpp::RetrieveRelevantHeaders() is converting it to Unicode and saving it away, but never retrieving it AFAICT. https://bugzilla.mozilla.org/buglist.cgi?quicksearch=content-disposition&list_id=279606 bz, do you know what that code is for? >--- a/content/base/src/nsDocument.cpp >@@ -6813,24 +6813,21 @@ nsDocument::RetrieveRelevantHeaders(nsIChannel *aChannel) >- nsCOMPtr<nsIMultiPartChannel> partChannel = do_QueryInterface(aChannel); >- if (partChannel) { >- nsCAutoString contentDisp; >- rv = partChannel->GetContentDisposition(contentDisp); >- if (NS_SUCCEEDED(rv) && !contentDisp.IsEmpty()) { >- SetHeaderData(nsGkAtoms::headerContentDisposition, >- NS_ConvertASCIItoUTF16(contentDisp)); >- } >+ nsCAutoString contentDisp; >+ rv = aChannel->GetContentDisposition(contentDisp); >+ if (NS_SUCCEEDED(rv)) { >+ SetHeaderData(nsGkAtoms::headerContentDisposition, >+ NS_ConvertASCIItoUTF16(contentDisp)); One more little thing: >+ // RFC 2183, section 2.8 says that an unknown disposition >+ // value should be treated as "attachment" >+ // If all of these tests eval to false, then we have a content-disposition of >+ // "attachment" or unknown >+ return dispToken.IsEmpty() || >+ dispToken.LowerCaseEqualsLiteral("inline") || >+ // Broken sites just send >+ // Content-Disposition: filename="file" >+ // without a disposition token... screen those out. >+ StringHead(dispToken, 8).LowerCaseEqualsLiteral("filename") || >+ // Also in use is Content-Disposition: name="file" >+ StringHead(dispToken, 4).LowerCaseEqualsLiteral("name"); So we treat these broken versions as inline? I see it's in the old code too--I'm just surprised they aren't considered attachment. Content-Disposition: filename="file" Content-Disposition: name="file"

Pardon the random link to https://bugzilla.mozilla.org/buglist.cgi?quicksearch=content-disposition&list_id=279606 in the last comment. It's a link to all the open bugs that mention 'content-disposition'. There's 217 of them! It'd be nice to see which are fixed by this work, and figure out if there's a common issue with any that aren't fixed.

> It seems preferable to me to have a separate 'nsIContentDispositionChannel' > interface, We could do that, but that's extra interfaces to implement in a bunch of places... It's a good question, I agree; I have no problem hanging it off of nsIChannel, though. We have lots of stuff there already that doesn't apply to many channels. I agree that better functions for getting the "actual disposition" would be nice. I don't see that as a reason to not hand out the whole header; we should do it in addition to that, instead. > nsDocument.cpp::RetrieveRelevantHeaders() is converting it to Unicode and > saving it away, but never retrieving it AFAICT. > bz, do you know what that code is for? Yes, because I went and looked at the blame... For what it's worth, it's retrieved in at least saveDocument in contentAreaUtils.js, which wants to get the filename to save to.

> So we treat these broken versions as inline? Yes, we do. Again, blame should be able to tell you why.

(bz: yes, I'll look at blame more often): So the overarching issue for me in this bug is that we centralize our C-D parsing. We want to eventually be compliant with all the cases in http://greenbytes.de/tech/tc2231/ (at least the subset that we feel don't break backward compatibility). We don't want to duplicate/maintain that effort in multiple code locations. Right now even with this patch we've still got JS code that's rolling its own parsing logic of the C-D header, ex: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#851 I initially thought that having the nsIChannel properties would be enough here, but we've got code that's stuffing the entire C-D header into places like the image cache and nsIDOMWindowUtils and then retrieving it in contexts where the channel itself is apparently(?) not available: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#158 http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#182 The context menu popupstate contains full C-D header too: http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/ContextCommands.js#62 I'm really not thrilled about exposing the full C-D header outside the channel--it seems like it just makes it likely that subtle bugs will pop up (like bug 480100). In bug 224209 comment 13 bz talked about changing the imgcache to take pre-parsed disposition/filename parameters instead of the raw C-D header. That's the approach I'd like to see us take, long-term, everywhere the C-D data is used. It that too ambitious? Short-term, we've got this patch, which I think is close, and is incremental progress, and would be good to land. I propose we change it to have nsIChannel have the following XPCOM APIs: int contentDisposition; // enum: NOT_PROVIDED, INLINE, ATTACHMENT string contentDispositionFilename; // the file name string contentDispositionHeader; // the full, raw Content-Disposition header, // marked "don't use this for new code: // will be deprecated soon" and at least have the code in nsFeedSniffer::HasAttachmentDisposition() and nsDocumentOpenInfo::DispatchContent() use the 'contentDisposition' field. (I'm not clear on whether it's better to have a NOT_PROVIDED value, or throw an exception with NS_ERROR_NOT_AVAILABLE: thoughts? We do need to distinguish between 'not-provided/invalid' and 'attachment' in order to fix bug 272541 and many of the test cases at greenbytes.com: right now NS_ContentDispositionIsInline doesn't do that: but again, I'm ok with that being a followup). I pondered also adding some sort of "GetFilenameFromContentDisposition()" function to nsIMimeHeaderParam.idl, so that JS functions that currently hand-parse the raw C-D header could at least have access to a common implementation (JS has no access to NS_GetFilenameFromDisposition()). But I think the better long-term solution is to move JS away from using raw C-D, and I'd rather not add an XPCOM function and then remove it later. re: testing: it would be great to have some of the greenbytes.de test cases in our xpcshell tests.

> It that too ambitious? Long-term, no. Short-term, you addressed. ;) We might want nsINetUtil methods for parsing content-disposition, so JS channel impls can do the right thing too, perhaps. I'm not sure there's a need to distinguish "not provided" and "attachment". All callers care about is "inline, or not?", really. Lack of header should be treated as "inline", imo. For the bug 272541 cases, why can't we just report "inline" if that's the behavior we want?

> All callers care about is "inline, or not?", really. You're right. We can just have DISPOSITION_INLINE and DISPOSITION_ATTACHMENT enums for the 'contentDisposition' parameter. > We might want nsINetUtil methods for parsing content-disposition, so > JS channel impls can do the right thing too, perhaps. If we've got time to squeeze that into this bug, that'd be bonus points.

Nick, did the discussion above derail this patch? :(

(In reply to comment #42) > Nick, did the discussion above derail this patch? :( More that I just got sidetracked by the necko perf testing stuff. I kind of need a break from that anyway, so I'll go ahead and get the 3-property version of this going.

New version, now with 3 properties (see the previous few comments)! Even though it's mostly the same as the previous version, re-requesting r & sr since I feel like it's changed "enough" to warrant it (whatever that means...)

Comment on attachment 543301 [details] [diff] [review] patch v4 Seems like the nsNetUtil includes are no longer needed some places (e.g. nsFeedSniffer). >+ PRUint32 mContentDisposition; >+ nsCString mContentDispositionHeader; These will pack better with the other jar channel members if you switch the order. Also, document that mContentDisposition has an uninitialized value unless mContentDispositionHeader is nonempty. For the nsIChannel change, I would prefer we put the new stuff at the end of the interface. > +HttpBaseChannel::GetContentDispositionHeader(nsACString& Truncating before returning error in the !mResponseHead case is probably not a good idea. Just take that truncate call out? Every single consumer of NS_ContentDispositionIsInline actually wants to convert the boolean to the enum value. I think it would be better to have an NS_GetContentDisposition function (or two, one for channel+cstring and one for astring) that returns the enum, and then make NS_ContentDispositionIsInline just compare the return value of that to nsIChannel::DISPOSITION_INLINE. sr=me with those changes.

Updates for bz's comments, carry forward sr+

Comment on attachment 544102 [details] [diff] [review] patch v4.1 Let's hold off on reviewing this... it seems as if a recent bitrot update results in this patch preventing the browser chrome from appearing (at least in debug builds on os x), and causes some sort of infinite loop where we try to display the chrome, fail, try again, etc.

Apparently I had a horrible logic error, causing the chrome to not appear. Fixed that. The new version also has the happy result of fixing bug 272541 all on its own (we should look at 272541 and see if there's anything else we want to bring in from it independently, though). All of Julian's tests behave the same as on a regular Nightly build (with the exception of the case covered by 272541, of course), so nothing got unduly broken. Also noticed a couple instances of really crappy/wrong return values in some code I wrote, so I fixed those. Re-requesting r and sr even though it's a somewhat minor change, just to make sure I didn't do anything else stupid.

Comment on attachment 549227 [details] [diff] [review] patch v4.2 The two places you added that are returning INLINE should be returning ATTACHMENT, no?

The first one (when we fail trying to get mimehdrpar) I could see going either way, since that's an internal error and what we do at that point isn't defined by any spec. The second one (when we fail trying to get the first parameter in the content-disposition header) seems to me to map to the case of an empty content-disposition header (either non-existent or some horribly broken server sent "Content-Disposition: "), in which case returning INLINE seems to be the right thing to do. Am I missing something in the second case? (And do we want to change the first case, or handle our internal error in some other way?)

> Am I missing something in the second case? Yes, all the discussion in bug 272541. That applies to the first case too.

This is what I get for not sleeping last night... new version of the patch is incoming once I get a few cycles to re-do it again. (We'll still need bug 272541 with these fixes.)

Fix inline/attachment default. Makes bug 272541 relevant again, too.

Comment on attachment 549268 [details] [diff] [review] patch v4.3 Review of attachment 549268 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, if I understood all preconditions correctly. r=honzab with following comments addressed. ::: modules/libjar/nsJARChannel.h @@ +96,5 @@ > nsCOMPtr<nsISupports> mListenerContext; > nsCString mContentType; > nsCString mContentCharset; > + nsCString mContentDispositionHeader; > + PRUint32 mContentDisposition; /* This is uninitialized if mContentDispositionHeader is empty */ 80 lines please. Put the comment above the member and change "This" to "mContentDisposition". ::: netwerk/base/public/nsNetUtil.h @@ +1927,5 @@ > + * @param header the content-disposition header (full value) > + */ > +inline PRUint32 > +NS_GetContentDisposition(nsIChannel *chan, const nsACString& header) > +{ Make |chan| argument optional? And maybe change names to aArgument form in all your new functions? @@ +1999,5 @@ > + aFilename); > + if (NS_FAILED(rv) || aFilename.IsEmpty()) > + // Try 'name' parameter, instead. > + rv = mimehdrpar->GetParameter(aDisposition, "name", fallbackCharset, > + PR_TRUE, nsnull, aFilename); Put this multiline code under if to { } block ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +379,5 @@ > > NS_IMETHODIMP > +HttpBaseChannel::GetContentDisposition(PRUint32 *aContentDisposition) > +{ > + nsresult rv = NS_OK; Why initializing to NS_OK ? @@ +396,5 @@ > +HttpBaseChannel::GetContentDispositionFilename(nsAString& aContentDispositionFilename) > +{ > + aContentDispositionFilename.Truncate(); > + > + nsresult rv = NS_OK; Same here ::: netwerk/streamconv/converters/nsMultiMixedConv.h @@ +75,5 @@ > nsresult SendOnStartRequest(nsISupports* aContext); > nsresult SendOnDataAvailable(nsISupports* aContext, nsIInputStream* aStream, > PRUint32 aOffset, PRUint32 aLen); > nsresult SendOnStopRequest(nsISupports* aContext, nsresult aStatus); > + void SetContentDisposition(const nsACString& aContentDisposition); Please comment what value is expected in the argument, or at least rename it.

(In reply to comment #54) > ::: netwerk/base/public/nsNetUtil.h > @@ +1927,5 @@ > > + * @param header the content-disposition header (full value) > > + */ > > +inline PRUint32 > > +NS_GetContentDisposition(nsIChannel *chan, const nsACString& header) > > +{ > > Make |chan| argument optional? While I'm not opposed to this (even though it's not necessary right now), is there a particular reason for wanting to make it optional? It makes it more clear which version of the function is being called, at least in the current state where we have 1-argument and 2-argument functions with the same name. Granted, there's an argument to be made for getting rid of the current 1-argument version of the function (it's not currently used, and I just kept it for some twisted sake of completeness). So perhaps that's the right route to take? (We could even make the chan argument optional in this case without causing undue confusion.)

Comment on attachment 549268 [details] [diff] [review] patch v4.3 sr=me

(In reply to comment #55) > While I'm not opposed to this (even though it's not necessary right now), is > there a particular reason for wanting to make it optional? It is passed to extract an URI to get fallback charset for the parser. The URI argument is optional in NS_GetFilenameFromDisposition and actually in NS_GetContentDisposition too, because you don't fail when there is no URI on the channel. But you require the channel. So, I just wanted to be consistent with what we already have. > It makes it more > clear which version of the function is being called, at least in the current > state where we have 1-argument and 2-argument functions with the same name. Yes. And that is exactly one of reasons why I really dislike function overload. The difference is that one function expects the whole header and other just the disposition token (the first part before ; char). Actually hard to recognize from the arguments required. > Granted, there's an argument to be made for getting rid of the current > 1-argument version of the function (it's not currently used, and I just kept > it for some twisted sake of completeness). So perhaps that's the right route > to take? (We could even make the chan argument optional in this case without > causing undue confusion.) Hope I follow correctly :) but maybe just name the two functions NS_GetContentDispositionFromHeader and NS_GetContentDispositionFromToken, or rename this way only the function expecting just the token. It will clearly say what is the expected argument.

Update to address comments. Carry forward r+ and sr+

Should probably be pushed to try before being checked in on mozilla-inbound...

I noticed that NS_GetContentDispositionFromHeader is not consistent on indention. It should use 2 space indention. Should be updated before landing. Going to push this to try and land if all OK.

Fixes messed up indentation in NS_GetContentDispositionFromHeader, carry forward r+ & sr+

(In reply to Honza Bambas (:mayhemer) from comment #62) > http://tbpl.mozilla.org/?tree=Try&rev=19cc6108331d Regular failure: TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/modules/libjar/test/unit/test_bug589292.js | test failed (with xpcshell return code: 0), see following log: TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIChannel.open]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/modules/libjar/test/unit/test_bug589292.js :: run_test :: line 12" data: no]

(In reply to Honza Bambas (:mayhemer) from comment #64) > (In reply to Honza Bambas (:mayhemer) from comment #62) > > http://tbpl.mozilla.org/?tree=Try&rev=19cc6108331d > > Regular failure: > > TEST-UNEXPECTED-FAIL | > /home/cltbld/talos-slave/test/build/xpcshell/tests/modules/libjar/test/unit/ > test_bug589292.js | test failed (with xpcshell return code: 0), see > following log: > TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component > returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIChannel.open]" > nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: > /home/cltbld/talos-slave/test/build/xpcshell/tests/modules/libjar/test/unit/ > test_bug589292.js :: run_test :: line 12" data: no] Did you add the zip file also attached to the bug in modules/libjar/test/unit/data/test_bug589292.zip ? I probably should've made it more clear that that was part of what's necessary since the patch doesn't handle binary files properly.

(In reply to Nick Hurley from comment #65) > Did you add the zip file also attached to the bug in > modules/libjar/test/unit/data/test_bug589292.zip ? I probably should've made > it more clear that that was part of what's necessary since the patch doesn't > handle binary files properly. Please make that file part of the patch. This is actually a reason to give r- to the patch ; I wasn't aware. $hg add modules/libjar/test/unit/data/test_bug589292.zip $hg qrefresh If you have correctly set .hgignore then hg stat should _before adding the file_ show: $hg stat ? modules/libjar/test/unit/data/test_bug589292.zip after 'hg add' should be: $hg stat A modules/libjar/test/unit/data/test_bug589292.zip and after 'hg grefresh' should 'hg stat' give no output.

Patch with the zipfile included

Jorge, This is going to add 3 new fields to nsIChannel (see comment 39 to 41, and the nsIChannel.idl part of the patch), and remove a field from nsIMultiPartChannel.idl (I doubt very much that it's being used by Add-ons). This work provides a "better way" to get content-disposition information from a necko channel (right now clients grab the header and have to parse it themselves, which is hazardous for various reasons). We certainly want to encourage add-ons to use the new fields rather than manually get the HTTP header (though we'll never remove the ability to do it that way). Honza/Nick: try is green, but I want to look over the patch one more time, so don't land it yet.

Some minor nits. Nick, let me know if you're ok with them. Marked nsIChannel::contentDispositionHeader as deprecated. I'll file bug on removing it. Got rid of comments about needing to factor out code from nsFeedSniffer.cpp, Removed NS_ContentDispositionTokenIsInline and NS_ContentDispositionHeaderIsInline: unused. HttpBaseChannel::GetContentDispositionHeader: we need to check for failure from GetHeader, else if there is no c-d header, but argument passed in is not empty, you'll return OK. I didn't change them, but I suspect that we don't actually want to pass in the Channel to NS_GetContentDispositionFromHeader (and friends): it appears we may not want to try to use the Channels' fallback charset (or pass TRUE for aTryLocaleCharset) in nsMIMEHeaderParamImpl::GetParameter (in the case of GetDisposition, the only permissible values are ascii (attachment/inline) anyway: for filenames, non-ascii should specify the charset in the parameter? See bug 685192). For now let's land and figure this out later.

remove test_bug589292.js (we don't want to name tests after bug #s anymore: use descriptive name). Folded into a new test_headers file that we can use to conglomerate header tests (the fewer individual xpcshell files we have the faster the tests will run: avoid httpd.js startup, etc).

Comment on attachment 559334 [details] [diff] [review] patch on top of v4.6: minor issues Looks good, didn't know about "@deprecated", good to know.

Comment on attachment 559335 [details] [diff] [review] changes to test code (another patch on top of v4.6) Review of attachment 559335 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, presuming the tests work (don't have my ssh key for try, nor a full build system on this machine).

Added e10s version of xpcshell test, too (FYI: all necko xpcshell tests should have a _wrap version in netwerk/tests/unit_ipc) http://hg.mozilla.org/integration/mozilla-inbound/rev/3208bcc1eaf9 Changing name, since this is not e10s-specific.

(In reply to Jason Duell (:jduell) from comment #74) > http://hg.mozilla.org/integration/mozilla-inbound/rev/3208bcc1eaf9 On m-c now

Comment on attachment 553203 [details] [diff] [review] patch v4.6 >+ if (NS_SUCCEEDED(rv) && disp == nsIChannel::DISPOSITION_ATTACHMENT) >+ return PR_TRUE; > > return PR_FALSE; ...

Neil, did you have something more to say in comment 76?