Closed Bug 955550 Opened 10 years ago Closed 10 years ago

Abstract Buddy Request Code Into jsProtoHelper.js

Categories

(Chat Core :: Yahoo! Messenger, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: qheaden, Assigned: qheaden)

Details

Attachments

(1 file, 3 obsolete files)

*** 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.
Attached patch Patch 1 (obsolete) — Splinter Review
*** 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.
Attachment #8354506 - Flags: review?(clokep)
Attachment #8354506 - Flags: review?(florian)
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
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 :-).
Attachment #8354506 - Flags: review?(florian) → review-
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.
Attachment #8354506 - Flags: review?(clokep) → review+
Attached patch Patch 2 (obsolete) — Splinter Review
*** 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.
Attachment #8354533 - Flags: review?(florian)
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 ***
Attachment #8354506 - Attachment is obsolete: true
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...
Attachment #8354533 - Flags: review?(florian) → review-
Attached patch Patch 3 (obsolete) — Splinter Review
*** Original post on bio 2112 as attmnt 2773 at 2013-08-23 23:18:00 UTC ***

This patch addresses the previous review comments given.
Attachment #8354542 - Flags: review?(florian)
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 ***
Attachment #8354533 - Attachment is obsolete: true
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.
Attachment #8354542 - Flags: review?(florian) → review-
Attached patch Patch 4Splinter Review
*** 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.
Attachment #8354563 - Flags: review?(florian)
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 ***
Attachment #8354542 - Attachment is obsolete: true
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 ***
Attachment #8354563 - Flags: review?(florian) → review+
*** 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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.