From Bug 505192 comment #28 (In reply to comment #26) > > Can you use NetUtil.readInputStreamToString here? > > NetUtil.readInputStreamToString doesn't set character encoding. It is no > good when we need character conversion. We could fix the NetUtil with an optional argument, I'm sure sdwilsh could review that change.

Comment on attachment 533565 [details] [diff] [review] fix Drive-by review: >+ if (aCharset) { >+ let cis = Cc["@mozilla.org/intl/converter-input-stream;1"]. >+ createInstance(Ci.nsIConverterInputStream); >+ try { >+ cis.init(aInputStream, "UTF-8", aCount, 0); I think you want to use aCharset here instead of "UTF-8". I would also suggest to test at least one other charset in the unit test to ensure the charset is actually configurable.

Comment on attachment 535275 [details] [diff] [review] fix Should I define NS_ERROR_ILLEGAL_INPUT in xpc.msg?

Comment on attachment 535275 [details] [diff] [review] fix Thanks for patch! I'm not a toolkit peer, 302ing the review to sdwilsh. That said, here's some feedback: >+ * @throws NS_ERROR_ILLEGAL_INPUT if aInputStream has invalid sequenses (typo in "sequences") >+ do_check_eq(e.result, 0x8050000E); // NS_ERROR_ILLEGAL_INPUT (In reply to comment #4) > Should I define NS_ERROR_ILLEGAL_INPUT in xpc.msg? Yes, that's probably a good idea, assuming this would expose it in Components.results. And then change the above line to use Cr.NS_ERROR_ILLEGAL_INPUT. >+ do_check_eq(NetUtil.readInputStreamToString(istream, >+ TEST_DATA_UTF8.length, >+ "UTF-8", >+ Ci.nsIConverterInputStream. >+ DEFAULT_REPLACEMENT_CHARACTER), nit: indention

I'm not sure I like adding two optional arguments here. What about using an object here for the optional arguments?

That seems fine to me.

Comment on attachment 535597 [details] [diff] [review] fix v2 Clearing request until comment 7 is addressed.

(In reply to comment #7) > I'm not sure I like adding two optional arguments here. What about using an > object here for the optional arguments? Should I define NetUtil_readInputStreamToString(aInputStream, aCount, aOptions) instead of? aReplacementChar is for replacing some code with this. On bug 505192, we should trow invalid UTF-8 sequence, but other usages uses replacement character such as http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStartup.js#318.

(In reply to comment #10) > Should I define NetUtil_readInputStreamToString(aInputStream, aCount, aOptions) > instead of? Yes

Comment on attachment 538819 [details] [diff] [review] fix v3 Review of attachment 538819 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh with changes address. You should get sr from bz. ::: netwerk/base/src/NetUtil.jsm @@ +273,5 @@ > * @param aInputStream > * The input stream to read from. > * @param aCount > * The number of bytes to read from the stream. > + * @param aOption [optional] nit: aOptions @@ +316,5 @@ > + let cis = Cc["@mozilla.org/intl/converter-input-stream;1"]. > + createInstance(Ci.nsIConverterInputStream); > + try { > + if (!aOption.replacement) { > + // throw NS_ERROR_ILLEGAL_INPUT if invalid sequences This comment doesn't seem very useful in its current form. Can you try to rephrase it please? @@ +325,5 @@ > + let str = {}; > + cis.readString(-1, str); > + cis.close(); > + return str.value; > + } catch (e) { nit: local style says the catch starts on the next line after the } ::: netwerk/test/unit/test_NetUtil.js @@ +573,5 @@ > + NetUtil.readInputStreamToString(istream, > + TEST_DATA_UTF8.length, > + { charset: "UTF-8" }); > + do_throw("should throw!"); > + } catch (e) { nit: catch should be on the line after } @@ +574,5 @@ > + TEST_DATA_UTF8.length, > + { charset: "UTF-8" }); > + do_throw("should throw!"); > + } catch (e) { > + do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_INPUT); Can we test that the stack is fixed to the right call site too?

(In reply to comment #13) > @@ +574,5 @@ > > + TEST_DATA_UTF8.length, > > + { charset: "UTF-8" }); > > + do_throw("should throw!"); > > + } catch (e) { > > + do_check_eq(e.result, Cr.NS_ERROR_ILLEGAL_INPUT); > > Can we test that the stack is fixed to the right call site too? This exception should throws by uconv. I believe that stack won't fixed forever.

Comment on attachment 547026 [details] [diff] [review] v4 >+ * @param aOptions [optional] >+ * charset >+ * The character encoding of stream data. >+ * replacement >+ * The character to repelace unknown byte sequences. >+ * If unset, it causes an exceptions to be thrown. s/repelace/replace/ and please indent the descriptions of the options a bit with respect to the option nam. The existing comment is very hard to read. >+ if (aOptions && aOptions.charset) { Shouldn't that test be |aOptions && "charset" in aOptions|? Or at least do the "in" test before doing the boolean test if you really want to test for nonempty charset. >+ if (!aOptions.replacement) { >+ // aOptions.replacement isn't set. >+ // If input stream has unknown sequences for aOptions.charset, >+ // throw NS_ERROR_ILLEGAL_INPUT. >+ aOptions.replacement = 0; Again, this feels like it should be an "in" test, not a boolean test (e.g. aOptions may already have "replacement: 0"). sr=me with those changes.

(In reply to Makoto Kato from comment #15) > This exception should throws by uconv. I believe that stack won't fixed > forever. We can catch it in NetUtil code and then set the stack correctly.

Documentation updated: en/JavaScript_code_modules/NetUtil.jsm#readInputStreamToString() And mentioned on Firefox 11 for developers.