Add an API to asynchronously copy data to an output stream

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Networking
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: sdwilsh, Assigned: bz)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.2a1
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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?
(Assignee)

Comment 2

9 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?
I think network utility functions should live under netwerk/. Toolkit is a container for too much random stuff already.
(Assignee)

Comment 4

9 years ago
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
(Assignee)

Comment 7

9 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

9 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)

Updated

8 years ago
Blocks: 485976
(Assignee)

Comment 9

8 years ago
Created attachment 373358 [details] [diff] [review]
Proposed patch

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)
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["@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

8 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

8 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
Last Resolved: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Assignee)

Comment 13

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/3b1fb9fce222 with the comment and const change.
(Reporter)

Updated

8 years ago
Blocks: 491936
(Reporter)

Comment 14

8 years ago
Tests added in bug 491936.
Flags: in-testsuite? → in-testsuite+
(Reporter)

Updated

8 years ago
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:

https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm
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.