589025 - e10s: ExternalHelperAppParent must support content-disposition property of nsIChannel

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Brian Crowder]

Attachment: Add support for Content-Disposition: header across process boundaries

Summary+patch says it all.

Comment 1

[[e-mail]]

We can also fix this by just moving contentDisposition onto nsIChannel -- bug 589292. Let's see what happens there, because it'll basically give you what you want for free. ;)

Comment 2

[Brian Crowder]

We should merge this patch with the patch in that bug, then, since that bug has ExternalHelperAppParent returning an empty Content-Disposition, which is not sufficient...  would that be alright?

Comment 3

[[e-mail]]

Yep!

Comment 4

[[e-mail]]

Comment on attachment 467625 [details] [diff] [review]
Add support for Content-Disposition: header across process boundaries

Canceling request -- want to write a patch on top of the one in that bug, and I'll stamp?

Comment 5

[Brian Crowder]

Heading out to go see my kids after their first day of school, but I'll happily spin a rev of this tomorrow morning built on the patch in bug 589292 (you going to spin that for read-only?)

Comment 6

[Brian Crowder]

Attachment: Add support for Content-Disposition: header across process boundaries

Comment 7

[Brian Crowder]

This now requires the work from bug 589292 and will not work/apply without it.

Comment 8

[Brian Crowder]

Comment on attachment 469168 [details] [diff] [review]
Add support for Content-Disposition: header across process boundaries

If you like this, maybe you can just land it in-sync with the patch from bug 589292?

Comment 9

[[e-mail]]

Comment on attachment 469168 [details] [diff] [review]
Add support for Content-Disposition: header across process boundaries

> void
> ExternalHelperAppParent::Init(TabParent *parent,
>                               const nsCString& aMimeContentType,
>+                              const nsCString& aContentDisposition,
>                               const PRBool& aForceSave)
> {

>+  SetContentDisposition(aContentDisposition);

Latest patch in that bug makes contentDisposition readonly, so you'll want to directly set mContentDisposition here.

> NS_IMETHODIMP
> ExternalHelperAppParent::SetContentDisposition(const nsACString& aContentDisposition)
> {
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  mContentDisposition = aContentDisposition;
>+  return NS_OK;
> }

Likewise you'll want to remove this.

r=dwitte with that. Want to post an exported patch?

Comment 10

[Brian Crowder]

Attachment: the one to land

Keeping dwitte's review

Comment 11

[[e-mail]]

http://hg.mozilla.org/projects/electrolysis/rev/bc6739931542

Should leave this open til we merge to m-c. Should do that before Monday.

Comment 12

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/bc6739931542

Comment 13

[[e-mail]]

Need a new fix here now that bug 589292 is getting backed out.

Comment 14

[[e-mail]]

This should block fennec2.0b1 but not beta5, right?

Comment 15

[Benjamin Smedberg]

Yes.

Comment 16

[Brian Crowder]

Yes, this should block Fennec 2.0b1 because it is required functionality for Content-Disposition to work correctly.  The original patch is the "new fix" needed here.  I'll refresh that patch if it needs it and if not re-request review.

Comment 17

[Brian Crowder]

Attachment: the older version, updated

With r=dwitte in the checkin-message, I'll update in review moves.

Comment 18

[[e-mail]]

Comment on attachment 471539 [details] [diff] [review]
the older version, updated

>+NS_IMETHODIMP
>+ExternalHelperAppParent::GetBaseChannel(nsIChannel* *aChannel)
>+{
>+  *aChannel = this;

Any reason you didn't just make this throw? If not, I'd prefer that.

r=dwitte

Comment 19

[Brian Crowder]

Attachment: land this

Carrying dwitte's review over with GetBaseChannel change

Comment 20

[Brian Crowder]

Attachment: no, land -this-

Comment 21

[Josh Matthews [:jdm]]

http://hg.mozilla.org/mozilla-central/rev/4906e07c0970