955550 - Abstract Buddy Request Code Into jsProtoHelper.js

Status: RESOLVED FIXED

(Chat Core :: Yahoo! Messenger, enhancement)

Description

[Quentin Headen [:qheaden]]

*** Original post on bio 2112 at 2013-08-16 18:58:00 UTC ***

Both JS-XMPP and JS-Yahoo use similar code to send buddy requests to the UI and respond to user actions. This functionality needs to be abstracted into jsProtoHelper to prevent code duplication.

Comment 1

[Quentin Headen [:qheaden]]

Attachment: Patch 1

*** Original post on bio 2112 as attmnt 2737 at 2013-08-20 01:20:00 UTC ***

This patch adds a showBuddyRequest() method to GenericAccountPrototype. I also made some changes to JS-Yahoo and JS-XMPP to make use of this new method and reduce code duplication.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8354506 [details] [diff] [review]
Patch 1

*** Original change on bio 2112 attmnt 2737 at 2013-08-20 08:10:16 UTC ***

>+  showBuddyRequest: function(aUserName, aGrantCallback, aDenyCallback,
>+                             aCancelCallback) {
>+    // We allow an optional cancel callback. This callback is optional since
>+    // some prpls only need to submit a notification and do nothing else.
>+    let cancelCallback;

This is confusing to me. "cancel" isn't exposed in the idl interface, it's not a callback that the UI can call, but a method that the prpl can call to remove the notification from the UI.
All prpls need to do it when a request is no longer relevant (which is always the case if the account gets disconnected). JS-XMPP does it at http://lxr.instantbird.org/instantbird/source/chat/protocols/xmpp/xmpp.jsm#1199 :
1199     for each (let request in this._pendingAuthRequests)
1200       request.cancel();
1201     this._pendingAuthRequests = [];

>+    if (aCancelCallback)
>+      cancelCallback = aCancelCallback;
>+    else {
>+      cancelCallback = function() {
>+        Services.obs.notifyObservers(this,
>+                                     "buddy-authorization-request-canceled",
>+                                     null);

Why isn't this just a method of the authRequest object?

>+    let authRequest = {
>+      get account() this,

Not sure this line actually works (I would have expected |get account() this.jsAccount.imAccount|). But maybe you tested it? :)

>+      get userName() aUserName,
>+      jsAccount: this,

I think we usually name this _account.

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm

>+  /* Replies to a buddy request in order to accept it or deny it. */
>+  replyToBuddyRequest: function(aRequest, aReply) {

If you swap the order of these 2 parameters, you can bind the first parameter (aReply) to the value "subscribed" or "unsubscribed" and get rid of acceptBuddyRequest and denyBuddyRequest.

>+    this._pendingAuthRequests =
>+      this._pendingAuthRequests.filter(function(r) r !== aRequest);

This should be done in jsProtoHelper.

And I think the cleanup when accounts are disconnected should also happen from jsProtoHelper.

>diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm

I won't pretend I understand the yahoo specific part, so I'll assume clokep will review it :-).

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 8354506 [details] [diff] [review]
Patch 1

*** Original change on bio 2112 attmnt 2737 at 2013-08-20 12:22:52 UTC ***

(In reply to comment #2)
> >diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm
> 
> I won't pretend I understand the yahoo specific part, so I'll assume clokep
> will review it :-).

I agree with Florian's other comments, I don't have any comments (currently) on the Yahoo parts.

Comment 4

[Quentin Headen [:qheaden]]

Attachment: Patch 2

*** Original post on bio 2112 as attmnt 2764 at 2013-08-23 17:26:00 UTC ***

This patch addresses the feedback given by Florian. It abstracts the handling of pending requests into jsProtoHelper.js. The removal of pending requests on a disconnect has also been abstracted in this patch.

Comment 5

[Quentin Headen [:qheaden]]

Comment on attachment 8354506 [details] [diff] [review]
Patch 1

*** Original change on bio 2112 attmnt 2737 at 2013-08-23 17:26:03 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8354533 [details] [diff] [review]
Patch 2

*** Original change on bio 2112 attmnt 2764 at 2013-08-23 20:28:13 UTC ***

>diff --git a/chat/modules/jsProtoHelper.jsm b/chat/modules/jsProtoHelper.jsm

>   // Called when the user adds a new buddy from the UI.
>+  _pendingBuddyRequests: [],

This should be initialized in an _init method, otherwise all objects that share this prototype will use the same array.


>+  cancelPendingBuddyRequests: function() {
>+    for each(let request in this._pendingBuddyRequests)

Nit: space after "each".

>+      request.cancel();

This method needs to cleanup/reset this._pendingBuddyRequests.


>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm

>+  /* Replies to a buddy request in order to accept it or deny it. */
>+  replyToBuddyRequest: function(aReply, aRequest) {
>+    let connection = this._connection;

Nit: This variable doesn't have much value.

>+    if (!connection)
>+      return;
>+    connection.sendStanza(Stanza.presence({to: aRequest.userName,
>+                                           type: aReply}));

If the connection variable was only for the length of this line, then I would rather do:

      let s = Stanza.presence({to: aRequest.userName, type: aReply});



>+      let authRequest = this.addBuddyRequest(jid,
>+                                             this.replyToBuddyRequest
>+                                                 .bind(this, "subscribed"),
>+                                             this.replyToBuddyRequest
>+                                                 .bind(this, "unsubscribed"));

The line breaks before .bind are a little strange. Would this look better if you put a line break after the =, like this:
let authRequest =
  this.add...

Comment 7

[Quentin Headen [:qheaden]]

Attachment: Patch 3

*** Original post on bio 2112 as attmnt 2773 at 2013-08-23 23:18:00 UTC ***

This patch addresses the previous review comments given.

Comment 8

[Quentin Headen [:qheaden]]

Comment on attachment 8354533 [details] [diff] [review]
Patch 2

*** Original change on bio 2112 attmnt 2764 at 2013-08-23 23:18:27 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8354542 [details] [diff] [review]
Patch 3

*** Original change on bio 2112 attmnt 2773 at 2013-08-23 23:55:17 UTC ***

>   // Called when the user adds a new buddy from the UI.

This comment was meant to stay immediately above the addBuddy method.

>+  _pendingBuddyRequests: null,
>+  get pendingBuddyRequests() this._pendingBuddyRequests,

Is there any code calling this getter?

>   addBuddy: function(aTag, aName) {


>+  cancelPendingBuddyRequests: function() {
>+    for each (let request in this._pendingBuddyRequests)
>+      request.cancel();
>+    this._pendingBuddyRequests = [];

In such a case we would usually do:
 delete this._pendingBuddyRequests;

But this wouldn't work for you without other changes, as you initialize to a new array only when initializing the account; not when connecting it.

What about creating the array in addBuddyRequest if it was null (rather than in _init)? removeBuddyRequest and cancelPendingBuddyRequests would then need a null check too.

Comment 10

[Quentin Headen [:qheaden]]

Attachment: Patch 4

*** Original post on bio 2112 as attmnt 2793 at 2013-08-27 23:14:00 UTC ***

This addresses the feedback given. I removed the getter to access the _pendingBuddyRequests array since it wasn't being accessed at all. Accesses to _pendingBuddyRequests shouldn't need to happen (at least not now) since all of the handling for the requests are handled internally.

Comment 11

[Quentin Headen [:qheaden]]

Comment on attachment 8354542 [details] [diff] [review]
Patch 3

*** Original change on bio 2112 attmnt 2773 at 2013-08-27 23:14:21 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8354563 [details] [diff] [review]
Patch 4

*** Original change on bio 2112 attmnt 2793 at 2013-08-28 00:04:43 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[Florian Quèze [:florian]]

*** Original post on bio 2112 at 2013-08-28 00:06:02 UTC ***

http://hg.instantbird.org/instantbird/rev/bae538c64373

As discussed on IRC, I moved a few lines around in jsProtoHelper to increase readability before the check-in.