Closed Bug 482310 Opened 16 years ago Closed 15 years ago

Add an API to asynchronously copy data to an output stream


(Core :: Networking, defect)

Not set





(Reporter: sdwilsh, Assigned: bzbarsky)



(Keywords: dev-doc-complete)


(1 file)

Originally from a dev.platform thread.  I'd like to see an API that allows us to asynchronously write data to a file, and bz suggested this:
OK.  So what you want here, basically, is a function that takes your data in some form and takes an output stream (notably NOT an nsIFile or filename) and does the async copy, right?  This should be pretty simple to write, and I'm happy to do so if someone points me to the right place to dump it.... ;) 

In terms of error handling, I'd by happy with the ability to register a function that receives error information, and handles it accordingly (much like how mozIStorageStatementCallback::handleError works).

This probably isn't the right component, but I'm not sure where the right one is.  This felt like the best place to put it.
How would this differ from NS_AsyncCopy or the nsIAsyncStreamCopier?
The idea here is to have a simple-to-use JS-only API.  It'd basically be a thin shim around nsIAsyncStreamCopier.

The right component for this is whatever toolkit component is correct for "toolkit utility functions", no?
I think network utility functions should live under netwerk/. Toolkit is a container for too much random stuff already.
OK, the question remains of how to expose the JS function to consumers usefully...
I recommend Cu.import("resource://gre/network.js")
hm, sure, though nsIAsyncStreamCopier isn't really hard to use
It takes 10-20 lines of boilerplate for what can (and should, and is in C++) be a single function call.
mimicking nsNetUtil.h for JS would probably be incredibly useful.  Of course, it can't have the same signature, but similar is good too.
Blocks: 485976
Attached patch Proposed patchSplinter Review
This has tests for the C++ class I added, but not yet for the .jsm.  With any luck, Shawn will write some of the latter.  ;)

I didn't expose the full API that nsNetUtil or the interface exposes (complete with being able to provide an event target, being able to control closing behavior, etc).  We could add some optional arguments for all that, with an exception thrown if we have to create a buffered wrapper for a stream the caller wants kept open, because otherwise there might be data stuck in the buffers that the caller doesn't know about...
Assignee: nobody → bzbarsky
Attachment #373358 - Flags: superreview?(benjamin)
Attachment #373358 - Flags: review?(dcamp)
Attachment #373358 - Flags: review?(benjamin)
Attachment #373358 - Flags: superreview?(benjamin)
Attachment #373358 - Flags: superreview+
Attachment #373358 - Flags: review?(dcamp)
Attachment #373358 - Flags: review?(benjamin)
Attachment #373358 - Flags: review+
Comment on attachment 373358 [details] [diff] [review]
Proposed patch

>diff --git a/netwerk/base/src/NetUtil.jsm b/netwerk/base/src/NetUtil.jsm

>+ * Necko utilities

nit, extra * in header

>+var NetUtil = {

any reason not to make this 'const' as well?

>+        if (!sourceBuffered && !sinkBuffered) {
>+            // wrap the sink in a buffered stream.
>+            var bostream = Cc[";1"]
>+                             .createInstance(Ci.nsIBufferedOutputStream);

My personal style preference for new code is to put operators at the end of the line and not to line up dots:

            var bostream = Cc[";1"].

But biesi or you should make that final decision.
> any reason not to make this 'const' as well?

Hmm.  Probably not.  I just copy/pasted from XPCOMUtils.jsm here.  ;)

As far as operators, I was following what I understood to be our general JS style.  I personally prefer the style you describe as well, and would be happy to switch to it!
Pushed, but forgot to address the '*' and const nits.  I'll do that once the tree reopens.

Filed bug 491936 on writing the tests.
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Pushed with the comment and const change.
Blocks: 491936
Tests added in bug 491936.
Flags: in-testsuite? → in-testsuite+
Keywords: dev-doc-needed
Target Milestone: --- → mozilla1.9.2a1
I haven't written this up yet, but I've reorganized the code modules content to create a good place to put it. It will go here when I write it, which should be very shortly:
You need to log in before you can comment on or make changes to this bug.