*** Original post on bio 1251 at 2012-01-30 11:33:00 UTC *** - No add request pops up. - The "Add buddy" dialog doesn't do anything for JS-XMPP accounts. This doesn't matter for Facebook Chat as the server doesn't support it from XMPP, but it's a serious regression for Gtalk users.

*** Original post on bio 1251 as attmnt 1175 at 2012-02-16 18:31:00 UTC *** This patch doesn't handle showing invitations, as the current code for that (in purplexpcom) is so wrong that I'll need to fully redesign/rewrite it before working on the XMPP part.

*** Original post on bio 1251 at 2012-02-16 19:48:58 UTC *** Comment on attachment 8352922 [details] [diff] [review] (bio-attmnt 1175) Patch, part 1 - add/remove buddies >diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm >@@ -143,19 +143,20 @@ XMPPSession.prototype = { > execHandler: function(aId, aStanza) { > if (!this._handlers.hasOwnProperty(aId)) >- return; >- this._handlers[aId](aStanza); >+ return false; >+ let handler = this._handlers[aId](aStanza); > this.removeHandler(aId); >+ return true; > }, Why are we storing handler here? >diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm >@@ -354,16 +354,28 @@ const XMPPAccountBuddyPrototype = { > > return new nsSimpleEnumerator(tooltipInfo); > }, > > get normalizedName() this.userName, > /* Display name of the buddy */ > get contactDisplayName() this.buddy.contact.displayName || this.displayName, > >+ remove: function() { >+ if (!this._account.connected) >+ return; >+ >+ let s = Stanza.iq("set", null, null, >+ Stanza.node("query", Stanza.NS.roster, null, >+ Stanza.node("item", null, >+ {jid: this.normalizedName, >+ subscription: "remove"}))); >+ this._account._connection.sendStanza(s); >+ }, In addBuddy you throw if disconnected, here you just return early. That seems a bit strange to me. Does the UI let us remove a buddy if the account is offline? (Of course we should still check, but I'm curious.) >+ addBuddy: function(aTag, aName) { [...] >+ if (this._buddies.hasOwnProperty(jid)) { >+ DEBUG("not re-adding an existing buddy, should check if the subscription is OK though... :(\n"); >+ return; >+ } Should this have an TODO comment? The rest looks OK at first glance, but I'll look over it again.

*** Original post on bio 1251 at 2012-02-16 20:06:33 UTC *** (In reply to comment #2) > Comment on attachment 8352922 [details] [diff] [review] (bio-attmnt 1175) [details] > Patch, part 1 - add/remove buddies > > >diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm > >@@ -143,19 +143,20 @@ XMPPSession.prototype = { > > execHandler: function(aId, aStanza) { > > if (!this._handlers.hasOwnProperty(aId)) > >- return; > >- this._handlers[aId](aStanza); > >+ return false; > >+ let handler = this._handlers[aId](aStanza); > > this.removeHandler(aId); > >+ return true; > > }, > Why are we storing handler here? It's a left-over from something I wrote before. At some point I wanted: let handler = ... this.removeHandler(aId); return handler(aStanza); But then I decided I didn't want to go change all the handlers to return a boolean. > >diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm > >@@ -354,16 +354,28 @@ const XMPPAccountBuddyPrototype = { > > > > return new nsSimpleEnumerator(tooltipInfo); > > }, > > > > get normalizedName() this.userName, > > /* Display name of the buddy */ > > get contactDisplayName() this.buddy.contact.displayName || this.displayName, > > > >+ remove: function() { > >+ if (!this._account.connected) > >+ return; > >+ > >+ let s = Stanza.iq("set", null, null, > >+ Stanza.node("query", Stanza.NS.roster, null, > >+ Stanza.node("item", null, > >+ {jid: this.normalizedName, > >+ subscription: "remove"}))); > >+ this._account._connection.sendStanza(s); > >+ }, > In addBuddy you throw if disconnected, here you just return early. That seems a > bit strange to me. Does the UI let us remove a buddy if the account is offline? > (Of course we should still check, but I'm curious.) I think throwing in addBuddy will prevent the dialog from closing. In the remove method, I think it's mostly a matter of how much noise we want to make if this isn't done correctly. The "Remove" context menu item isn't disabled for disconnected accounts; I'm not sure what the expected behavior is in that case. > >+ addBuddy: function(aTag, aName) { > [...] > >+ if (this._buddies.hasOwnProperty(jid)) { > >+ DEBUG("not re-adding an existing buddy, should check if the subscription is OK though... :(\n"); > >+ return; > >+ } > Should this have an TODO comment? Maybe: DEBUG("not re-adding an existing buddy"); // TODO check if the subscription is ok or if we should resend a presence request.

Comment on attachment 8352922 [details] [diff] [review] Patch, part 1 - add/remove buddies *** Original change on bio 1251 attmnt 1175 at 2012-02-16 21:10:14 UTC *** r+ with the changes from comment 3.

*** Original post on bio 1251 as attmnt 1178 at 2012-02-20 15:36:00 UTC *** Second part; this diff applies above part 1 which should be applied first.

Comment on attachment 8352927 [details] [diff] [review] Patch, part 2 - authorization requests *** Original change on bio 1251 attmnt 1178 at 2012-02-20 16:47:55 UTC *** The JS parts of this all look fine. The only comments I have is whether the changes to blist.js should be else if instead of just if statements. I didn't look over the libpurple code in depth, but it looks OK on the surface.

*** Original post on bio 1251 at 2012-02-20 16:56:30 UTC *** (In reply to comment #6) > The only comments I have is whether the > changes to blist.js should be else if instead of just if statements. All this method looks like: if (aSubject == "some-value") { // Do something... return; } The only instance that doesn't look like this is: else if (aTopic == "showing-ui-conversation") (visible in the context of the patch you reviewed). To improve consistency we could either remove this |else| (as the code block above it finishes with a |return|) or change the whole method to this style: if (aSubject == "some-value) { // Do something... else if (aSubject == "some-other-value) { // Do something else... } else if (aSubject == ...

*** Original post on bio 1251 at 2012-02-20 17:05:22 UTC *** I think it'd be better if the styled matched, but I won't block this landing based on that. (Removing the else is probably sufficient.)

*** Original post on bio 1251 at 2012-02-21 10:21:57 UTC *** Fixed: Part 1: https://hg.instantbird.org/instantbird/rev/bce36aedaa74 Part 2: https://hg.instantbird.org/instantbird/rev/aeeb3eb04a8e