Closed
Bug 955550
Opened 10 years ago
Closed 10 years ago
Abstract Buddy Request Code Into jsProtoHelper.js
Categories
(Chat Core :: Yahoo! Messenger, enhancement)
Chat Core
Yahoo! Messenger
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: qheaden, Assigned: qheaden)
Details
Attachments
(1 file, 3 obsolete files)
11.90 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Assignee | ||
Comment 1•10 years ago
|
||
*** 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)
Assignee | ||
Updated•10 years ago
|
Attachment #8354506 -
Flags: review?(florian)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
*** 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)
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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-
Assignee | ||
Comment 7•10 years ago
|
||
*** 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)
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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-
Assignee | ||
Comment 10•10 years ago
|
||
*** 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)
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
*** 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.
Description
•