Closed
Bug 482310
Opened 16 years ago
Closed 15 years ago
Add an API to asynchronously copy data to an output stream
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: sdwilsh, Assigned: bzbarsky)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
22.78 KB,
patch
|
benjamin
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•16 years ago
|
||
How would this differ from NS_AsyncCopy or the nsIAsyncStreamCopier?
Assignee | ||
Comment 2•16 years ago
|
||
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?
Comment 3•16 years ago
|
||
I think network utility functions should live under netwerk/. Toolkit is a container for too much random stuff already.
Assignee | ||
Comment 4•16 years ago
|
||
OK, the question remains of how to expose the JS function to consumers usefully...
Comment 5•16 years ago
|
||
I recommend Cu.import("resource://gre/network.js")
Comment 6•16 years ago
|
||
hm, sure, though nsIAsyncStreamCopier isn't really hard to use
Assignee | ||
Comment 7•16 years ago
|
||
It takes 10-20 lines of boilerplate for what can (and should, and is in C++) be a single function call.
Reporter | ||
Comment 8•16 years ago
|
||
mimicking nsNetUtil.h for JS would probably be incredibly useful. Of course, it can't have the same signature, but similar is good too.
Assignee | ||
Comment 9•16 years ago
|
||
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
Status: NEW → ASSIGNED
Attachment #373358 -
Flags: superreview?(benjamin)
Attachment #373358 -
Flags: review?(dcamp)
Attachment #373358 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #373358 -
Flags: superreview?(benjamin)
Attachment #373358 -
Flags: superreview+
Attachment #373358 -
Flags: review?(dcamp)
Attachment #373358 -
Flags: review?(benjamin)
Attachment #373358 -
Flags: review+
Comment 10•15 years ago
|
||
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["@mozilla.org/network/buffered-output-stream;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["@mozilla.org/network/buffered-output-stream;1"].
createInstance(Ci.nsIBufferedOutputStream);
But biesi or you should make that final decision.
Assignee | ||
Comment 11•15 years ago
|
||
> 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!
Assignee | ||
Comment 12•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/55339f8d68fa, but forgot to address the '*' and const nits. I'll do that once the tree reopens.
Filed bug 491936 on writing the tests.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Assignee | ||
Comment 13•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/3b1fb9fce222 with the comment and const change.
Reporter | ||
Updated•15 years ago
|
Keywords: dev-doc-needed
Target Milestone: --- → mozilla1.9.2a1
Comment 15•15 years ago
|
||
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:
https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm
Comment 16•15 years ago
|
||
OK, this is now documented here:
https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•