Closed Bug 795442 Opened 7 years ago Closed 7 years ago

Allow nsIChannel.contentDispositionFilename to be writable

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jduell.mcbugs, Assigned: evilpie)

References

Details

Attachments

(1 file, 1 obsolete file)

v1
22.53 KB, patch
jduell.mcbugs
: review+
Biesinger
: superreview+
Details | Diff | Splinter Review
biesi and I discussed on IRC that the best way to support bug 676619 (and possibly bug 665216) is to make nsIChannel.contentDispositionFilename writable (rather than using channel as a propertyBag, which is gross and will hopefully be eliminated Any Day Now).

I'm guessing this will need e10s support (i.e. ParentChannel will need to set it on the "real" necko channel), because the 'download' attribute will be set by HTML parser, but file access needs to happen in parent. True? I suppose it depends on the mechanics that are used.  It would be a bit of a shame to schlep an empty string across IPDL for the 99.99% of channels that don't use this, but I guess it's not much overhead.
Attached patch WIP (obsolete) — Splinter Review
I added SetContentDisposition and SetContentDispositionFilename. nsBaseChannel and HttpBaseChannel have a special implementation that actually does something. This allows me to implement Bug 676619 for http resources and blobs. Currently this implementation forces the values set on HttpBaseChannel instead of allowing the actual header values.

I don't know what I am supposed to do about the mentioned e10s support.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Comment on attachment 666199 [details] [diff] [review]
WIP

Review of attachment 666199 [details] [diff] [review]:
-----------------------------------------------------------------

Biesi:  I assume you're OK with also making nsIChannel.contentDisposition writable as well? (see nsIChannel.idl--I think that's all you need to look at in this patch).

Tom: Good stuff--thanks.  Make the following changes and I'll +r the next revision of this patch.

::: netwerk/base/public/nsIChannel.idl
@@ +257,5 @@
>       * Implementations should throw NS_ERROR_NOT_AVAILABLE if the header either
>       * doesn't exist for this type of channel or is empty, and
>       * DISPOSITION_ATTACHMENT if an invalid/noncompliant value is present.
>       */
> +    attribute unsigned long contentDisposition;

Update the UUID of nsIChannel.idl since we're changing the IDL, i.e. change the 'uuid' macro right above 'interface nsIChannel' to this (for instance: I just generated a new uuid for you):

  1d5927b0-0c08-11e2-892e-0800200c9a66

::: netwerk/base/src/nsBaseChannel.cpp
@@ +511,5 @@
>  
>  NS_IMETHODIMP
>  nsBaseChannel::GetContentDispositionFilename(nsAString &aContentDispositionFilename)
>  {
> +  aContentDispositionFilename = mContentDispositionFilename;

To preserve previous behavior, return NS_ERROR_NOT_AVAILABLE if mContentDispositionFilename.Length() == 0, i.e. if the param hasn't been overridden.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +28,5 @@
>  
>  HttpBaseChannel::HttpBaseChannel()
> +  : mContentDispositionOverriden(false)
> +  , mContentDisposition(DISPOSITION_INLINE)
> +  , mStartPos(LL_MAXUINT)

It's better to put your new variables somewhere other than the start of the list, so you preserve hg blame for the mStartPos line.  Not a big deal.  If you move them, make sure to keep the order of declaration in .h and .cpp constructor list the same.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +235,5 @@
>    nsCString                         mContentTypeHint;
>    nsCString                         mContentCharsetHint;
>    nsCString                         mUserSetCookieHeader;
>  
> +  bool                              mContentDispositionOverriden;

rename mContentDispositionIsOverridden (note two 'd's in Overidden :)

@@ +237,5 @@
>    nsCString                         mUserSetCookieHeader;
>  
> +  bool                              mContentDispositionOverriden;
> +  uint32_t                          mContentDisposition;
> +  nsString                          mContentDispositionFilename;

Both here and in nsBaseChannel I'd prefer to name these 'mContentDispositionOverride' and 'mContentDispositionFilenameOverride'.  I don't want anyone glancing at these variable names and assuming they represent the normal case.
Attachment #666199 - Flags: superreview?(cbiesinger)
Attachment #666199 - Flags: feedback+
(In reply to Jason Duell (:jduell) from comment #2)
> To preserve previous behavior, return NS_ERROR_NOT_AVAILABLE if
> mContentDispositionFilename.Length() == 0, i.e. if the param hasn't been
> overridden.

Use IsEmpty() instead of checking Length
Comment on attachment 666199 [details] [diff] [review]
WIP

Review of attachment 666199 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsBaseChannel.cpp
@@ +60,5 @@
>    , mWasOpened(false)
>    , mWaitingOnAsyncRedirect(false)
>    , mStatus(NS_OK)
> +  , mContentDispositionOverriden(false)
> +  , mContentDisposition(DISPOSITION_INLINE)

Instead of using two variables, why not use a value of -1 to indicate that it hasn't been overridden?

I guess this is unsigned, so PR_UINT32_MAX would be better than -1

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +387,5 @@
>  HttpBaseChannel::GetContentDispositionFilename(nsAString& aContentDispositionFilename)
>  {
>    aContentDispositionFilename.Truncate();
>  
> +  if (!mContentDispositionFilename.IsEmpty()) {

Hmm, in general I think that a C-D header should override the download attribute. That's also how other nsIChannel attributes behave (contentType, contentCharset). Is there a spec for this feature?
biesi is right: the spec here says that the HTTP headers should take precedence if set:

   http://developers.whatwg.org/links.html#downloading-resources

in that case, why don't we rename s/Override/Hint/ in the patch and only check the hint if the HTTP headers are missing.

I'm also fine with biesi's suggestion to use -1 and skip having the extra bool variable (I actually also considered suggesting that mContentDispositionFilename be a nsRefPtr<nsString>, which is 8 bytes instead of 16: do we care that much about saving 8 bytes?)
Attachment #666199 - Flags: superreview?(cbiesinger) → superreview-
Sorry, I mean nsAutoPtr<nsString> in the last comment, not nsRefPtr.   If we use it we'd just test it for null, and non-null means it's overridden.  In SetContentDispositionFilename just assign it a "new nsString(...)" and nsAutoPtr will take care of freeing it in the destructor.
Attached patch v1Splinter Review
Great feedback. I think i fixed everything like you suggested.
Attachment #666199 - Attachment is obsolete: true
Attachment #667027 - Flags: review?(jduell.mcbugs)
Comment on attachment 667027 [details] [diff] [review]
v1

Review of attachment 667027 [details] [diff] [review]:
-----------------------------------------------------------------

Landed on inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/18eb6c5d14e3

I added some text to nsIChannel.idl to clarify that the C-D header value takes precedence to hint (same verbiage for contentDispositionFilename too):

+     * Setting contentDisposition provides a hint to the channel about the
+     * disposition.  If a normal Content-Disposition header is present its
+     * value will always be used.  If it is missing the hinted value will
+     * be used if set.
+     *
Attachment #667027 - Flags: review?(jduell.mcbugs) → review+
Note that hints set in a channel on one process in e10s won't show the hint in the channel in the other process.   That's fine for now, as the hint is only used on the child process.   We can open a new bug for cross-process if we wind up needing it.
https://hg.mozilla.org/mozilla-central/rev/18eb6c5d14e3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Huhu that was fast \o/. But I usually prefer to push patches myself.
Blocks: 736324
You need to log in before you can comment on or make changes to this bug.