151867 - file channel should implement nsIUploadChannel [was: Network should support "file://" protocol]

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Charles Manske]

Currently, nsIWebBrowserPersist has to "special case" transferring files to a
"file://" destination. Thus support should be built into networking code as 
suggested by Darin. With this, standard networking
nsIWebProgressListener::onStateChange notifications will occur so we don't have to 
do ugly hacks as Composer Publishing did in bug 142171.

Comment 1

[Darin Fisher]

not critical for moz 1.1

Comment 2

[Darin Fisher]

Attachment: v1 patch

tested this patch using netwerk/test/TestUpload

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 94974 [details] [diff] [review]
v1 patch

+    if (aContentLength < 0) {
+	 nsresult rv = aStream->Available(&mUploadStreamLength);
+	 if (NS_FAILED(rv)) return rv;
+    }
+    else
+	 mUploadStreamLength = aContentLength;

This makes the assumption that all data must be present in the stream (returned
by available) when you call setUploadStream.  For example, there is no read to
EOF semantics.	Maybe it would be nice to say: here is a input stream - write
everything until eof.

Comment 4

[Darin Fisher]

perhaps, but this implementation follows the documentation in
nsIUploadChannel.idl to the letter (or at least that was my attempt).

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 94974 [details] [diff] [review]
v1 patch

maybe that should be changed.  until then, i guess this is good to go.

Comment 6

[Doug Turner (:dougt)]

oh, and btw, this is really good stuff - we could build a location independent
file manager based on our channel notions.

Comment 7

[Darin Fisher]

yup :-)

Comment 8

[Alec Flett]

Comment on attachment 94974 [details] [diff] [review]
v1 patch

sr=alecf

A few possibly irrelevant questions: does the use of mUploadStream to decide if
we're uploading or downloading prevent a channel from being both an upload and
a download stream? does that matter? (i.e. I could see composer wanting to
download a file, and then re-upload it - do they automatically get a new
channel?)

I'm sure you're all over this darin, but figured I'd ask just in case :)

Comment 9

[Darin Fisher]

alec: nsIChannel only supports the notion of fetching an URL.  that URL could
require an upload stream to fetch it, and the resulting document might be empty.
 that is the case with uploading to a file:// URL.  your listener only sees an
OnStartRequest followed by an OnStopRequest with either a success or failure
status code.  if they want to later read the document, then they'd need to open
a new channel to the URL and call AsyncOpen without setting an upload stream. 
so, setting the upload stream determines the kind of request.  it works the same
way with FTP and HTTP.

Comment 10

[Darin Fisher]

fixed-on-trunk

dougt, alecf: thanks for the speedy reviews!!

Comment 11

[benc]

How can I use testUpload to verify this change?

Comment 12

[Doug Turner (:dougt)]

did you try running testUpload?  The usage is printed:

usage: testupload.exe <url> <file-to-upload>

Comment 13

[David Epstein]

verified. Mozilla 1.3b Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b)
Gecko/20030108