It would be really handy if JS could read from input streams with embedded nulls. This patch includes a helper method on NetUtil (readInputStreamToString) to make this even easier for JS consumers (no xpcom).

This blocks bug 568634, which is a beta 5 blocker, which makes this a beta 5 blocker.

Comment on attachment 469145 [details] [diff] [review] v1.0 >+ * @param aBytes >+ * The number of bytes to read from the stream. I was debating on making this optional and just get whatever is available() if it's not specified. Would like to hear thoughts on this.

Comment on attachment 469145 [details] [diff] [review] v1.0 +++ b/netwerk/base/src/NetUtil.jsm + * Reads aBytes bytes from aInputStream into a string. Can you rename aBytes to aCount? I think that naming would be more intuitive. +++ b/netwerk/test/unit/test_NetUtil.js + const TEST_DATA = "this is a test string"; Shouldn't you use a string with null bytes? To set that into a string stream, do: let istream = Cc["@mozilla.org/io/string-input-stream;1"]. createInstance(Ci.nsISupportsCString); istream.data = TEST_DATA; + do_execute_soon(function() { Why execute_soon?

(In reply to comment #2) > >+ * @param aBytes > >+ * The number of bytes to read from the stream. > I was debating on making this optional and just get whatever is available() if > it's not specified. Would like to hear thoughts on this. Seems simple enough for the caller to specify stream.available()

(In reply to comment #3) > Can you rename aBytes to aCount? I think that naming would be more intuitive. I did this originally, but aBytes sounded better in the comments. I don't care either way though, so I can flip it back. > Shouldn't you use a string with null bytes? To set that into a string stream, > do: Ah, yes I should. Good catch. > Why execute_soon? smaller stacks when failures happen

Per comments

Seems like the WOULD_BLOCK case will first read all the data in the stream, then throw it away and raise WOULD_BLOCK. That seems broken, no?

(In reply to comment #7) > Seems like the WOULD_BLOCK case will first read all the data in the stream, > then throw it away and raise WOULD_BLOCK. That seems broken, no? If we haven't read all the data we are supposed to (which, AFAICT is the only time we would get WOULD_BLOCK), the contract says we throw anyway, right? This is also the same behavior as NS_ReadInputStreamToString, which I really just wanted to call, but it throws NS_ERROR_UNEXPECTED (which I could just change this new API to do).

The usual nsIInputStream contract is that if you can return any data at all, you return and and don't throw. NS_ReadInputStreaToString does in fact fail if you ask it to read more bytes than the stream is willing to hand out..... I suppose it's ok to do the same here, as long as the interface very clearly documents that this does NOT have the usual input stream contract.

(In reply to comment #9) > The usual nsIInputStream contract is that if you can return any data at all, > you return and and don't throw. > > NS_ReadInputStreaToString does in fact fail if you ask it to read more bytes > than the stream is willing to hand out..... I suppose it's ok to do the same > here, as long as the interface very clearly documents that this does NOT have > the usual input stream contract. I'm not sure on what the right behavior is here to be honest. At least with JS, you can't throw and return something, so I'm not sure how to convey that you have some data still without having an awkward API. But, if we go with the currently implemented behavior, ss this sufficient, or do you want more (suggestions welcome): >+ * @throws NS_ERROR_FAILURE if there are not enough bytes available to read >+ * aCount amount of data.

That seems fine to me.

Comment on attachment 469180 [details] [diff] [review] v1.1 r=me with that comment added and an apostrophe added in "caller's location".

Comment on attachment 469180 [details] [diff] [review] v1.1 +++ b/xpcom/io/nsIScriptableInputStream.idl + [size_is(aCount), retval] out ACString aString); Interesting, I didn't know size_is worked with ACString. Why not just make this: ACString readBytes(in unsigned long aCount);

(In reply to comment #12) > r=me with that comment added and an apostrophe added in "caller's location". This was documented at both locations, but is now using the wording in comment 10, which was how it was documented in NetUtil.jsm. Also fixed grammar error. (In reply to comment #13) > Interesting, I didn't know size_is worked with ACString. Why not just make > this: > ACString readBytes(in unsigned long aCount); I don't know why I did it that way in the first place. Updated to make it work this way instead.

per comments, ready for checkin

Documented: https://developer.mozilla.org/en/JavaScript/Code_modules/NetUtil.jsm#readInputStreamToString() And listed on Firefox 4 for developers.

Also mentioned on the doc page for nsIInputStream.

(In reply to comment #18) > Also mentioned on the doc page for nsIInputStream. Do you mean nsIScriptableInputStream?

Um. Yeah, that's what I mean. Hey, look over there! *pointing*