*** Original post on bio 1171 at 2011-11-16 16:43:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***

*** Original post on bio 1171 as attmnt 999 at 2011-11-16 16:43:00 UTC *** I've started working on the integration of the code Varuna wrote this summer. I took the code from https://github.com/vpj/xmpp-js, updated it for compatibility with the changes made in bug 954193 (bio 759), and added the build system changes to make this a patch rather than an add-on. At this points it's possible to create accounts and attempt to connect them, but I haven't been able to login to either Gtalk or Facebook.

*** Original post on bio 1171 as attmnt 1002 at 2011-11-17 21:37:00 UTC *** This WIP can connect to Google Talk. It's possible to have a conversation. There are still lots of details to fix. Connecting to Facebook is still not possible, something seems broken in the DIGEST-MD5 implementation.

Comment on attachment 8352741 [details] [diff] [review] WIP 1 *** Original change on bio 1171 attmnt 999 at 2011-11-17 21:37:45 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 as attmnt 1014 at 2011-11-23 16:40:00 UTC *** This can connect to Google Talk and Facebook. The behavior w.r.t the contact list and conversations APIs seems OK now. Still a lot of cleanup/reviewing work to do.

Comment on attachment 8352744 [details] [diff] [review] WIP 2 *** Original change on bio 1171 attmnt 1002 at 2011-11-23 16:40:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 as attmnt 1028 at 2011-11-26 01:22:00 UTC *** Almost done cleaning up. I still need to do another pass on all the code, but I think I'm done with the large refactoring.

Comment on attachment 8352757 [details] [diff] [review] WIP 3 *** Original change on bio 1171 attmnt 1014 at 2011-11-26 01:22:18 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 as attmnt 1034 at 2011-11-28 18:53:00 UTC *** It's getting increasingly difficult to find areas of this code that need to be cleaned up. I guess this is a good sign :).

Comment on attachment 8352770 [details] [diff] [review] WIP 4 *** Original change on bio 1171 attmnt 1028 at 2011-11-28 18:53:21 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 as attmnt 1037 at 2011-11-29 17:35:00 UTC *** I'm done cleaning up this code. :) I'll now focus on the missing features. Next on my todo list: - handling several resources connected at the same time for the same buddy (sending the messages to the right resource). - buddy tooltips - MUC support - setting a buddy icon (and display name?) - adding/removing/moving buddies (includes displaying buddy add requests) - l10n - handling DNS SRV requests - some more small bug fixes to socket.jsm

Comment on attachment 8352776 [details] [diff] [review] WIP 5 *** Original change on bio 1171 attmnt 1034 at 2011-11-29 17:35:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 at 2011-11-30 01:02:06 UTC *** Things I've noticed while "reviewing" this code. Some of the copyright years are very out of date and you (Florian) should probably add yourself to the contributors on most files. Some also have "mozilla.org" as the initial create (makefiles?). I like the (simplified) GTalk for now! It would be nice to have some comments in xmpp-authmechs (e.g. "Handle PLAIN authorization mechanism" above a method PlainAuth doesn't really mean much. :-/) I've more to look at still. :)

*** Original post on bio 1171 at 2011-11-30 10:21:23 UTC *** (In reply to comment #6) > Some of the copyright years are very out of date Ok. > and you (Florian) should probably add yourself to the > contributors on most files. I'm not sure if I'm florian @ ib.org or @moco in this context :-S. > I like the (simplified) GTalk for now! :) > It would be nice to have some comments in xmpp-authmechs Ok. ("Ok" here means "added to my todo list") Other things I've added in the todo list since yesterday: - implement the "Connection security" option we have discussed on IRC. - hide the irrelevant options in the gtalk override - recreate a facebook chat override.

*** Original post on bio 1171 as attmnt 1043 at 2011-11-30 16:33:00 UTC *** New WIP handling these 2 points from comment 5: > - handling several resources connected at the same time for the same buddy > (sending the messages to the right resource). > - buddy tooltips One more item for the todo list: - support idleness (both sending and receiving/handling the idle time).

Comment on attachment 8352779 [details] [diff] [review] WIP 6 *** Original change on bio 1171 attmnt 1037 at 2011-11-30 16:33:50 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 as attmnt 1044 at 2011-12-02 16:56:00 UTC *** Changes: - MUC support - correct time stamps for delayed messages (messages sent while offline and context message when entering a MUC) (XEP-0203) - ping support (XEP-0199) - fixed the escaping of the < > & characters - idleness support (XEP-0256)

Comment on attachment 8352785 [details] [diff] [review] WIP 7 *** Original change on bio 1171 attmnt 1043 at 2011-12-02 16:56:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 as attmnt 1054 at 2011-12-05 18:45:00 UTC *** Changes: - l10n - Handling of account specific options. - A few bug fixes... Almost ready to land. I still need to write a facebook.js "protocol plugin".

Comment on attachment 8352786 [details] [diff] [review] WIP 8 *** Original change on bio 1171 attmnt 1044 at 2011-12-05 18:45:33 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 as attmnt 1059 at 2011-12-05 23:28:00 UTC *** I think this is ready, so now is probably the last opportunity to review this code before it lands. Changes: - facebook - packaging - some more bug fixes More things for the todo list: - private messages with MUC participants - MUC topics.

Comment on attachment 8352796 [details] [diff] [review] WIP 9 *** Original change on bio 1171 attmnt 1054 at 2011-12-05 23:28:05 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 at 2011-12-06 03:31:04 UTC *** Comment on attachment 8352801 [details] [diff] [review] (bio-attmnt 1059) WIP 10 >diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm >-function purplePref(aName, aLabel, aType, aDefaultValue, aMasked) { >+function purplePref(aName, aOption) { > this.name = aName; // Preference name >- this.label = aLabel; // Text to display >- this.type = aType; ... >+ if ("masked" in aOption && aOption.masked) >+ this.masked = true; > } This looks much nicer then our current API. And I think will fix a bug I just found when trying to set "Use SSL" to true by default. >diff --git a/chat/protocols/facebook/facebook.js b/chat/protocols/facebook/facebook.js >+ connect: function() { >+ // We can't used this.onError because this._connection doesn't exist. "We can't use" >diff --git a/chat/protocols/xmpp/xmpp-authmechs.jsm b/chat/protocols/xmpp/xmpp-authmechs.jsm >+// This module exports XMPPAuthMechanisms that is an object containing >+// all the supported SALS authentication mechanisms. I think this is supposed to be "SASL"? >+// By default we currently support the PLAIN and the DIGEST-MD5 mechanisms. >+// Add-ons can add support for more auth mechanisms by adding them >+// in XMPPAuthMechanisms. :-D >diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm >+function XMPPSession(aHost, aPort, aSecurity, aJID, aPassword, aAccount) { >+ Cu.reportError(e); Should we use ERROR here instead of reportError? >+ Cu.reportError(e); And here. >diff --git a/chat/protocols/xmpp/xmpp-xml.jsm b/chat/protocols/xmpp/xmpp-xml.jsm >+const $NS = { Something about starting a variable with a $ bothers me, but I assume there's a good reason for it. >+ "private" : "jabber:iq:private", Why does only this key have quotes? >+ if (aAttr) { >+ for (let i = 0; i < aAttr.length; ++i) { >+ this.attributes[aAttr.getQName(i)] = aAttr.getValue(i); >+ } I would remove these braces to conform to the rest of the file. I got a bit farther without more comments. I'm planning to read more, but not tonight: sorry I couldn't get through all of it. :( (Don't block on me though!)

Comment on attachment 8352801 [details] [diff] [review] WIP 10 *** Original change on bio 1171 attmnt 1059 by mook.moz+bugs.instantbird AT gmail.com at 2011-12-06 06:16:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-12-06 06:16:55 UTC *** Comment on attachment 8352801 [details] [diff] [review] (bio-attmnt 1059) WIP 10 (from review of attachment 8352801 [details] [diff] [review] (bio-attmnt 1059)) I think should probably re-review at some point - getting kinda sleepy. My eyes pretty much glazed over during the XML goop. >diff --git a/chat/Makefile.in b/chat/Makefile.in >-PROTOCOLS = twitter overrides >+PROTOCOLS = \ no more overrides? (its makefile seems to still be written) >diff --git a/chat/components/src/imCore.js b/chat/components/src/imCore.js >@@ -348,22 +348,30 CoreService.prototype = { @@ > // This is a real error, the protocol is registered and failed to init. >- Cu.reportError(e); >+ let error = "failed to create an instance of " + cid + ": " + e; >+ dump(error + "\n"); >+ Cu.reportError(error); consider using Cu.reportError(e) instead (mostly because that gives a useful line number). no point changing the dump() though, that has no line numbers anyway >diff --git a/chat/modules/imXPCOMUtils.jsm b/chat/modules/imXPCOMUtils.jsm > function scriptError(aModule, aLevel, aMessage) { > // Only continue if we want to see this level of logging. > let logLevel = Services.prefs.getIntPref("purple.debug.loglevel"); I guess I need to rewrite that logging patch ;) > function l10nHelper(aChromeURL) >+ } catch (e) { >+ Cu.reportError(e); >+ dump("Failed to get " + aStringId + "\n"); >+ return aStringId; Hmm, no arguments there? (Having some parameters is usually useful) >diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm >-function purplePref(aName, aLabel, aType, aDefaultValue, aMasked) { >+function purplePref(aName, aOption) { consider writing a JavaDoc-style comment block describing the keys in aOptions that are recognized? >+ if (aOption.default === undefined || aOption.default === null) would this produce a strict warning if !("default" in aOptions)? > getOptions: function() { >+ for (let optionName in this.options) for (let [name, options] in Iterator(this.options)) ? >diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm >+ sendString: function(aString, aEncoding) { >+ let stream = converter.convertToInputStream(aString); >+ this._outputStream.writeFrom(stream, stream.available()); does this work right for very long strings? (does the converter eagerly encode all the text?) >+ onTransportStatus: function(aTransport, aStatus, aProgress, aProgressmax) { >+ let status = nsITransportEventSinkStatus[aStatus]; >+ this.log("onTransportStatus(" + (status || aStatus) +")"); I recommend (status || aStatus.toString(16)) > QueryInterface: function(iid) { XPCOMUtils.generateQI? (also the other new QI impls, e.g. XMPPParser) >diff --git a/chat/protocols/facebook/Makefile.in b/chat/protocols/facebook/Makefile.in >+# The Initial Developer of the Original Code is >+# Florian Qu?ze <florian@queze.net>. please check file encoding before checking in; this appears to be not UTF-8. (other files too.) >diff --git a/chat/protocols/gtalk/gtalk.js b/chat/protocols/gtalk/gtalk.js >+GTalkAccount.prototype = { >+ connect: function() { >+ if (this.prefs.prefHasUserValue("resource")) { >+ let resource = this.getString("resource"); did you mean to touch prefs here? >diff --git a/chat/protocols/xmpp/xmpp-authmechs.jsm b/chat/protocols/xmpp/xmpp-authmechs.jsm >+function md5hex(aString) { >+ return [toHexString(hash.charCodeAt(i)) for (i in hash)].join(""); alternatively, hash.split("").map(toHexString).join(""); >+ _generateResponse: function(aStanza) { >+ let data = {}; >+ data.username = this._username; >+ if (!("realm" in data)) >+ data.realm = ""; or you can initialize this above, let data = { realm: "" }; >diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm >+ sendStanza: function(aStanza, aCallback, aObject) { >+ if (aCallback) >+ this.addHandler(aStanza.attributes.id, aCallback, aObject); it might be easier to go with aCallback.bind(aObject) - not sure. >+ startStream: function() { >+ this._parser = new XMPPParser(this); >+ this.send("<?xml version=\"1.0\"?><stream:stream to=\"" + this._domain + >+ "\" xmlns=\"jabber:client\" xmlns:stream=\"http://etherx.jabber.org/streams\" version=\"1.0\">"); this will cleaner if you use ' instead of " to delimit the string, I think >diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm >+const XMPPMUCConversationPrototype = { >+ onPresenceStanza: function(aStanza) { >+ if (aStanza.attributes["type"] == "unavailable") { >+ delete this._participants[nick]; do you need to check first if the participant exists? (for the observer notification) >+ _cancelTypingTimer: function() { >+ if (this._typingTimer) { setTimeout _can_, in theory, return 0 as the handle, I think :( ("_typingTimer" in this) is probably better. >+ getTooltipInfo: function() { >+ let now = Math.floor(new Date() / 1000); Date.now() / 1000? (all the other places where you construct an empty date and divide, too) >+ _saveIcon: function(aPhotoNode) { >+ let ostream = FileUtils.openSafeFileOutputStream(file); >+ let buddy = this; >+ NetUtil.asyncCopy(istream, ostream, function(rc) { >+ if (Components.isSuccessCode(rc)) >+ buddy.buddyIconFilename = Services.io.newFileURI(file).spec; I think you also need to call ostream.finish() >+ onPresenceStanza: function(aStanza) { >+ let status = aStanza.getElement(["status"]); >+ let priority = aStanza.getElement(["status"]); is this correct? >+ this._resources[resource] = { no need to do funny names here, right?

*** Original post on bio 1171 at 2011-12-06 09:28:19 UTC *** >+ "list": {label: "Select option", default: "option2", >+ listValues: {"option1": "Default option", >+ "option2": "Other option"}} The labels for the options are mixed up now.

*** Original post on bio 1171 at 2011-12-06 09:42:45 UTC *** Lots of valuable comments, thanks! :)

*** Original post on bio 1171 at 2011-12-06 10:28:40 UTC *** (In reply to comment #12) > >diff --git a/chat/protocols/xmpp/xmpp-authmechs.jsm b/chat/protocols/xmpp/xmpp-authmechs.jsm > >+// By default we currently support the PLAIN and the DIGEST-MD5 mechanisms. > >+// Add-ons can add support for more auth mechanisms by adding them > >+// in XMPPAuthMechanisms. > :-D What's funny here? I've rephrased the last sentence of this comment. > >diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm > >+function XMPPSession(aHost, aPort, aSecurity, aJID, aPassword, aAccount) { > >+ Cu.reportError(e); > Should we use ERROR here instead of reportError? > > >+ Cu.reportError(e); > And here. I think Cu.reportError is better in this case but I'm not completely sure. ERROR will report the error with the location of the ERROR call as location information. If e is already a script error (rather than a simple string thrown by our code), it contains better location information which Cu.reportError keeps. > >diff --git a/chat/protocols/xmpp/xmpp-xml.jsm b/chat/protocols/xmpp/xmpp-xml.jsm > >+const $NS = { > Something about starting a variable with a $ bothers me, but I assume there's a > good reason for it. I didn't like it either. I'm changing $NS to Stanza.NS. > >+ "private" : "jabber:iq:private", > Why does only this key have quotes? Because private is a registered keyword of the JS language and (at the very least) my editor is unhappy with it being used unquoted here.

*** Original post on bio 1171 at 2011-12-06 10:43:38 UTC *** > a/chat/protocols/xmpp/xmpp-xml.jsm > 349 this._node = null; > 350 return; > 351 } > 352 > 353 let node = new XMLNode(this._node, aUri, aLocalName, aQName, aAttributes); > 354 if (this._node) > 355 this._node.addChild(node); > 356 > 357 this._node = node; > 358 }, I might misunderstand the context, but shouldn't it be "if (!this._node)" in 354?

*** Original post on bio 1171 at 2011-12-06 11:39:35 UTC *** (In reply to comment #13) > >- Cu.reportError(e); > >+ let error = "failed to create an instance of " + cid + ": " + e; > >+ dump(error + "\n"); > >+ Cu.reportError(error); > consider using Cu.reportError(e) instead (mostly because that gives a useful > line number). no point changing the dump() though, that has no line numbers > anyway This change is intentional: knowning the cid (which contains the prpl name) of which we couldn't create an instance is much more valuable than having the location of the exception thrown in a meaningless place anyway (if we can instantiate a prpl, the problem is most likely not in the core). If that file was already using the logging code defined in imXPCOMUtils, I could have used ERROR here. > > function l10nHelper(aChromeURL) > > >+ } catch (e) { > >+ Cu.reportError(e); > >+ dump("Failed to get " + aStringId + "\n"); > >+ return aStringId; > Hmm, no arguments there? (Having some parameters is usually useful) Not sure of what you mean. Do you mean the dump'ed message should contain the other arguments of the function? Or that we should attempt to insert the provided arguments in the value returned as a fallback? I don't think this would add much value, as the point of this change is mostly to identify faster which string identifiers have typos. Seeing the wrong id in the UI where a localized strings should be displayed + seeing the wrong id in the terminal is way more useful than the NS_ERROR_FAILURE returned by GetStringFromName that used to be the only thing visible in the error console, without good location information. > >+ if (aOption.default === undefined || aOption.default === null) > would this produce a strict warning if !("default" in aOptions)? No. undefined is a magic value that doesn't cause the warning (yeah, JS is that magical...). However, even if it did produce a warning, an option without default value is a bug in the code. The warnings I want to avoid is those that appear during normal execution. > >diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm > > >+ sendString: function(aString, aEncoding) { > > >+ let stream = converter.convertToInputStream(aString); > >+ this._outputStream.writeFrom(stream, stream.available()); > does this work right for very long strings? (does the converter eagerly encode > all the text?) I think it does. If you want to double check, the implementation is at http://mxr.mozilla.org/mozilla-central/source/intl/uconv/src/nsScriptableUConv.cpp#230 > >+ let status = nsITransportEventSinkStatus[aStatus]; > >+ this.log("onTransportStatus(" + (status || aStatus) +")"); > I recommend (status || aStatus.toString(16)) ok. I added a "0x" in there for additional clarity ;). > > QueryInterface: function(iid) { > XPCOMUtils.generateQI? (also the other new QI impls, e.g. XMPPParser) I'm somehow a bit reluctant to use that in files that don't use XPCOMUtils already :-S. > >diff --git a/chat/protocols/gtalk/gtalk.js b/chat/protocols/gtalk/gtalk.js > >+ if (this.prefs.prefHasUserValue("resource")) { > >+ let resource = this.getString("resource"); > did you mean to touch prefs here? What do you mean? (I haven't finished handling your comments, I'll continue after lunch)

*** Original post on bio 1171 at 2011-12-06 15:04:25 UTC *** (In reply to comment #13) > >diff --git a/chat/protocols/xmpp/xmpp-authmechs.jsm b/chat/protocols/xmpp/xmpp-authmechs.jsm > > >+function md5hex(aString) { > > >+ return [toHexString(hash.charCodeAt(i)) for (i in hash)].join(""); > alternatively, hash.split("").map(toHexString).join(""); Are you sure that will work? With hash.split("") you get an array of strings. The code you suggest to replace produces an array of numbers. > >diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm > >+ _cancelTypingTimer: function() { > >+ if (this._typingTimer) { > setTimeout _can_, in theory, return 0 as the handle, I think :( This setTimeout implementation is the one from imXPCOMUtils.jsm, and it returns an nsITimer instance. The associated clearTimeout implementation will definitely cause an error if given a null argument. > >+ _saveIcon: function(aPhotoNode) { > > >+ let ostream = FileUtils.openSafeFileOutputStream(file); > >+ let buddy = this; > >+ NetUtil.asyncCopy(istream, ostream, function(rc) { > >+ if (Components.isSuccessCode(rc)) > >+ buddy.buddyIconFilename = Services.io.newFileURI(file).spec; > I think you also need to call ostream.finish() NetUtil.asyncCopy takes care of it for us (it's actually done at http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStreamUtils.cpp#370). > >+ onPresenceStanza: function(aStanza) { > > >+ let status = aStanza.getElement(["status"]); > > >+ let priority = aStanza.getElement(["status"]); > is this correct? No :) > >+ this._resources[resource] = { > no need to do funny names here, right? What do you mean?

*** Original post on bio 1171 as attmnt 1061 at 2011-12-06 17:18:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8352801 [details] [diff] [review] WIP 10 *** Original change on bio 1171 attmnt 1059 at 2011-12-06 17:18:05 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1171 at 2011-12-06 18:05:46 UTC *** I pushed this (after fixing another few tiny details that Mic pointed out on IRC) as https://hg.instantbird.org/instantbird/rev/8a2df449facc Resolving as FIXED. We will handle the likely regressions and the not-yet-implemented features in follow up bugs. Thanks to all of you for the reviews! :)

*** Original post on bio 1171 at 2011-12-06 18:07:26 UTC *** (In reply to comment #21) > Resolving as FIXED. Actually doing it this time :).

*** Original post on bio 1171 at 2011-12-08 10:16:53 UTC *** Follow-up: https://hg.instantbird.org/instantbird/rev/51bef9d59539 - Fix removing the statusText of a JS-XMPP buddy.

Comment on attachment 8352803 [details] [diff] [review] Patch v11 (final? \o/) Review of attachment 8352803 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1,2 @@ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 FTR, all new code should use MPL2 - http://www.mozilla.org/MPL/headers/

(In reply to Magnus Melin from comment #36) > Comment on attachment 8352803 [details] [diff] [review] > Patch v11 (final? \o/) > > Review of attachment 8352803 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: chat/protocols/xmpp/xmpp.jsm > @@ +1,2 @@ > > +/* ***** BEGIN LICENSE BLOCK ***** > > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 > > FTR, all new code should use MPL2 - http://www.mozilla.org/MPL/headers/ This attachment was posted in 2011, see comment 31. This code has been in comm-central for a while, and has been relicensed to MPL2.