*** Original post on bio 1981 at 2013-05-29 13:29:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***

*** Original post on bio 1981 as attmnt 2463 at 2013-05-29 13:29:00 UTC *** The API for binary data in socket.jsm was kind of under developed when we committed that code. After some of my oscar work I've come up with an API that actually works. OSCAR and Yahoo! (one of our GSoC projects) will need to use the same API, so I split this work out into a separate patch, hopefully it works fine without the rest of the OSCAR changes! The new API doesn't auto-split after a certain number of bytes (which would require the messages to all be the same size...). The new API takes all the received data and throws it at a user-defined function (onBinaryDataReceived), this now returns the length of bytes that were handled by user code, those bytes are then destroyed in the buffer and we keep the unhandled bytes for next time. This also adds ArrayBufferUtils.jsm, which is now used in socket.jsm. (Although it looks like it should be used in more places than it currently is.)

Comment on attachment 8354230 [details] [diff] [review] Binary socket improvements *** Original change on bio 1981 attmnt 2463 at 2013-06-06 01:10:32 UTC *** I do wonder if it would make more sense to create an ArrayBuffer wrapper object) that does many of the nice things for you (instead of using global functions, etc.), this could be similar to https://github.com/dcodeIO/ByteBuffer.js Anyway, let me know if anything in here seems crazy. We'll need this for the Yahoo and OSCAR prpls.

Comment on attachment 8354230 [details] [diff] [review] Binary socket improvements *** Original change on bio 1981 attmnt 2463 at 2013-06-06 09:48:32 UTC *** I looked quickly and haven't seen anything crazy. The |throw Cr.NS_ERROR_NOT_IMPLEMENTED;| worry me a little bit, so even though it's likely a good change, I think it'll need a careful review of the current consummers of the API to feel confident about it.

Comment on attachment 8354230 [details] [diff] [review] Binary socket improvements *** Original change on bio 1981 attmnt 2463 at 2013-06-06 10:36:25 UTC *** We could certainly remove the throw Cr.NS_ERROR_NOT_IMPLEMENTED;, they were useful in testing (instead of just nothing happening, because I had forgotten to implement a method...I'd get an error in the console).

*** Original post on bio 1981 at 2013-06-25 20:42:45 UTC *** Comment on attachment 8354230 [details] [diff] [review] (bio-attmnt 2463) Binary socket improvements >+ return [view[i] for (i in view)]; You could check if "[value for (value of iterable)]" works where this pattern is used. I'm not sure if the order of the items is preserved by for...of though!

Comment on attachment 8354230 [details] [diff] [review] Binary socket improvements *** Original change on bio 1981 attmnt 2463 at 2013-06-27 23:12:22 UTC *** In comment 2 I had already more or less r+'ed the real code changes here. Mic's comment 4 seems an interesting suggestion though. I took some time today in the train to think about the |throw Cr.NS_ERROR_NOT_IMPLEMENTED;|s. I think you should either revert them all (because that's unrelated to this patch, and risky), or implement a better behavior. throwing in LOG or DEBUG is just unacceptable, because these methodes aren't really needed for proper functionning, but will definitely be called in places where stopping the execution would break things pretty badly. For other methods, it's less clear, but it seems a bunch of them are optional. I think it makes sense to be noisy (and by 'noisy' I don't necessarily mean throw, but report something in the Error Console) for the methods that are really required, or won't be called in cases where they aren't required. I think the only methods that are really in this case are: onDataReceived, onBinaryDataReceived, onConnectionClosed and sendPing.

*** Original post on bio 1981 as attmnt 2527 at 2013-06-28 00:31:00 UTC *** Takes into accounts Mic's comments and removes the throwing.

Comment on attachment 8354230 [details] [diff] [review] Binary socket improvements *** Original change on bio 1981 attmnt 2463 at 2013-06-28 00:31:58 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354295 [details] [diff] [review] Binary socket improvements v2 *** Original change on bio 1981 attmnt 2527 at 2013-06-28 23:04:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354295 [details] [diff] [review] Binary socket improvements v2 *** Original change on bio 1981 attmnt 2527 at 2013-06-29 15:32:12 UTC *** I suppose that my feedback was only requested because of the for...of suggestion earlier? That's now what I looked at for giving feedback. >+function ArrayBufferToBytes(aBuf) { >+ let view = new Uint8Array(aBuf); >+ return [value for (value of view)]; >+} That's what I meant :) >+function ArrayBufferToString(aData) { >+ let view = new Uint8Array(aData); >+ return [String.fromCharCode(view[i]) for (i in view)].join(""); The same pattern (... iterable[index] ... for (index in iterable)]) occurs here, >+function ArrayBufferToHexString(aData) { >+ let uint8 = Uint8Array(aData); >+ >+ return "0x" + >+ [("0" + uint8[i].toString(16)).slice(-2) for (i in uint8)].join(" "); and here, > sendBinaryData: function(/* ArrayBuffer */ aData) { >- this.LOG("Sending binary data data: <" + aData + ">"); >- >- let uint8 = Uint8Array(aData); >+ this.LOG("Sending binary data: <" + ArrayBufferToHexString(aData) + ">"); > > // Since there doesn't seem to be a uint8.get() method for the byte array >- let byteArray = []; >- for (let i = 0; i < uint8.byteLength; i++) >- byteArray.push(uint8[i]); >+ let uint8 = Uint8Array(aData); >+ let byteArray = [uint8[i] for (i in uint8)]; and here too.

Comment on attachment 8354295 [details] [diff] [review] Binary socket improvements v2 *** Original change on bio 1981 attmnt 2527 at 2013-07-02 21:40:59 UTC *** I decided ArrayBufferUtils should have tests.

*** Original post on bio 1981 as attmnt 2575 at 2013-07-11 23:15:00 UTC *** Now with tests.

Comment on attachment 8354295 [details] [diff] [review] Binary socket improvements v2 *** Original change on bio 1981 attmnt 2527 at 2013-07-11 23:15:58 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354343 [details] [diff] [review] Binary socket improvements v3 *** Original change on bio 1981 attmnt 2575 at 2013-07-14 13:55:05 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354343 [details] [diff] [review] Binary socket improvements v3 *** Original change on bio 1981 attmnt 2575 at 2013-07-14 17:41:44 UTC *** I'm not very familiar with this, so take my opinion with a grain of salt. You might also want to let flo have a look over this. There weren't any problems but only places where I think something could be shortened or in another way made a bit more readable. You will need to remove the unused import in ArrayBufferUtils though and that's also the only reason for r- here. Thanks a lot! > diff --git a/chat/modules/ArrayBufferUtils.jsm b/chat/modules/ArrayBufferUtils.jsm > +Components.utils.import("resource:///modules/imXPCOMUtils.jsm"); As far as I can see there's nothing used from imXPCOMUtils.jsm in this file, is it? > +function ArrayBufferToBytes(aBuf) { > + let view = new Uint8Array(aBuf); > + return [b for (b of view)]; > +} This should work but what about shortening it to function ArrayBufferToBytes(aBuf) [b for (b of new Uint8Array(aBuf))] ? > +function ArrayBufferToString(aData) { > + let view = new Uint8Array(aData); > + return [String.fromCharCode(b) for (b of view)].join(""); > +} Shorten this too? > +function ArrayBufferToHexString(aData) { > + let uint8 = Uint8Array(aData); > + > + return "0x" + [("0" + b.toString(16)).slice(-2) for (b of uint8)].join(" "); > +} The blank line is not needed here. What about calling the variable 'view' instead of 'uint8', like in the other functions, if you don't plan to remove them (and this one neither))? > diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm > sendBinaryData: function(/* ArrayBuffer */ aData) { > [...] > // Since there doesn't seem to be a uint8.get() method for the byte array > - let byteArray = []; > - for (let i = 0; i < uint8.byteLength; i++) > - byteArray.push(uint8[i]); > + let uint8 = Uint8Array(aData); > + let byteArray = [b for (b in uint8)]; As you're not using the uint8-array anywhere, I'd say this should use the shiny new ArrayBufferToBytes function here. > diff --git a/chat/modules/test/test_ArrayBufferUtils.js b/chat/modules/test/test_ArrayBufferUtils.js > +function do_check_arraybuffer_eq(a, b) { > + let res = a.byteLength == b.byteLength; > + > + print(a.byteLength + " " + b.byteLength); > + > + let viewA = new Uint8Array(a); > + let viewB = new Uint8Array(b); > + > + for (let i = 0; i < viewA.byteLength; ++i) > + res &= viewA[i] == viewB[i]; > + > + do_check_neq(res, 0); > +} Wouldn't the following code be more readable? for (let i = 0; i < viewA.byteLength; ++i) res = res && (viewA[i] == viewB[i]); > MIC do_check_true(res); > MIC} > +function do_check_array_eq(a, b) { > + let res = a.length == b.length; > + for (let i = 0; i < a.length; ++i) > + res &= a[i] == b[i]; > + > + do_check_neq(res, 0); > +} Same here.

*** Original post on bio 1981 as attmnt 2595 at 2013-07-14 21:22:00 UTC *** Meets the review comments and splits it to test one function per test. (As alluded to on IRC.)

Comment on attachment 8354343 [details] [diff] [review] Binary socket improvements v3 *** Original change on bio 1981 attmnt 2575 at 2013-07-14 21:22:07 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8354364 [details] [diff] [review] Binary socket improvements v4 *** Original change on bio 1981 attmnt 2595 at 2013-07-14 21:33:52 UTC *** Looks good, thanks for the changes!

*** Original post on bio 1981 at 2013-07-15 21:28:42 UTC *** http://hg.instantbird.org/instantbird/rev/c9d9c5809c78