Last Comment Bug 307769 - document the fact that XMLHttpRequest::Send doesn't support streams with headers
: document the fact that XMLHttpRequest::Send doesn't support streams with headers
Status: RESOLVED FIXED
: fixed1.8, regression
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Darin Fisher
: Ashish Bhatt
:
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
Depends on: 308484
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-09 12:30 PDT by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2012-07-21 09:09 PDT (History)
8 users (show)
asa: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch: fix comments (1.71 KB, patch)
2005-09-15 17:30 PDT, Darin Fisher
cbiesinger: review+
jst: superreview+
Details | Diff | Splinter Review
v1.1 patch (2.02 KB, patch)
2005-09-16 18:36 PDT, Darin Fisher
mtschrep: approval1.8b5+
Details | Diff | Splinter Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2005-09-09 12:30:33 PDT
XMLHttpRequest always passes a content type to SetUploadStream, which means that
any possibly existing headers in the input stream will be sent as data.

In 1.7.x, that was different:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp&rev=MOZILLA_1_7_BRANCH&cvsroot=/cvsroot&mark=1487#1483

We should maybe still allow streams with headers.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2005-09-09 13:00:25 PDT
Either that or fix the documentation accordingly...

In any case, we need to do one or the other for 1.8, imo.

Darin, bsmedberg, thoughts?
Comment 2 Benjamin Smedberg [:bsmedberg] 2005-09-09 13:13:18 PDT
How does this affect web authors?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2005-09-09 13:30:00 PDT
It doesn't, but it affects extension authors or any other UniversalXPConnect
code that's using XMLHttpRequest.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2005-09-14 23:34:05 PDT
When we fix this, we should make sure not to cause bug 308484 on the 1.8 branch.
Comment 5 Darin Fisher 2005-09-15 09:58:47 PDT
Why should we fix this?  Do we know of many extensions that try to set headers
at the top of their data stream?  If possible, we should drop support for
headers in the data stream.  It is a messy way of specifying headers.

It looks like the change was introduced by the patch for bug 246651.
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-15 11:54:46 PDT
at least nsIXMLHttpRequest.idl needs to be fixed, which says the headers need to
be specified in the stream.
Comment 7 Darin Fisher 2005-09-15 17:30:03 PDT
Created attachment 196262 [details] [diff] [review]
v1 patch: fix comments
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2005-09-15 21:47:39 PDT
I'd be fine with us changing the IDL appropriately, sure.  As long as our IDL
and impl are consistent and we have _a_ way to set headers, I'm happy not to do
it in the stream (which I agree is a messy way).
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-16 04:48:43 PDT
Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

+   *		  compatible with nsIUploadChannel.setUploadStream, and a

I think it should be made clearer that this function expects no headers in the
stream - nsIUploadChannel.setUploadStream can behave either way.

should it be mentioned that a content-type for the uploaded data can be set
using setRequestHeader, or is that obvious?
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-16 07:57:09 PDT
Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

sr=jst
Comment 11 Darin Fisher 2005-09-16 14:33:16 PDT
> I think it should be made clearer that this function expects no headers in the
> stream - nsIUploadChannel.setUploadStream can behave either way.
> 
> should it be mentioned that a content-type for the uploaded data can be set
> using setRequestHeader, or is that obvious?

good points.  i'll extend the comments to mention those.
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-16 15:42:23 PDT
Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

r=biesi with those comment changes.
Comment 13 Darin Fisher 2005-09-16 18:36:37 PDT
Created attachment 196381 [details] [diff] [review]
v1.1 patch

OK, here's the version that I checked in.  I would like to land this on the 1.8
branch for consistency.
Comment 14 Darin Fisher 2005-09-16 18:38:27 PDT
marking FIXED
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2005-09-16 21:11:23 PDT
Could we add a quick blurb about this change linked off
http://developer.mozilla.org/docs/Adapting_XUL_Applications_for_Firefox_1.5 ?
Comment 16 Mike Schroepfer 2005-09-19 14:39:44 PDT
Comment on attachment 196381 [details] [diff] [review]
v1.1 patch

Can you write a blurb for an update to devmo and send to cbeard/deb?
Comment 17 Darin Fisher 2005-09-19 16:30:01 PDT
fixed1.8
Comment 18 Darin Fisher 2005-09-20 10:28:24 PDT
(In reply to comment #15)
> Could we add a quick blurb about this change linked off
> http://developer.mozilla.org/docs/Adapting_XUL_Applications_for_Firefox_1.5 ?

Done

Note You need to log in before you can comment on or make changes to this bug.