Closed Bug 955503 Opened 10 years ago Closed 10 years ago

New conversation tab should suggest chat rooms

Categories

(Instantbird Graveyard :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

Attachments

(2 files, 29 obsolete files)

40.17 KB, patch
florian
: review+
Details | Diff | Splinter Review
8.59 KB, patch
clokep
: review+
Details | Diff | Splinter Review
*** Original post on bio 2066 at 2013-07-24 22:38:00 UTC ***

Some protocols provide a way to query the server for available chat rooms (e.g. IRC's LIST). These chat rooms should be displayed in the new conversation tab.
Attached patch WIP (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2636 at 2013-07-24 23:06:00 UTC ***

I figured it made sense to attach this here. I'm marking it as WIP, but I don't think there's /too/ much left to be done.
Attachment #8354405 - Flags: feedback?(benediktp)
Attachment #8354405 - Flags: feedback?(aleth)
Attachment #8354405 - Flags: feedback?(clokep)
Comment on attachment 8354405 [details] [diff] [review]
WIP

*** Original change on bio 2066 attmnt 2636 at 2013-07-24 23:07:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354405 - Attachment is patch: true
Attachment #8354405 - Attachment mime type: application/octet-stream → text/plain
Depends on: 955492
Comment on attachment 8354405 [details] [diff] [review]
WIP

*** Original change on bio 2066 attmnt 2636 at 2013-07-25 10:40:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354405 - Flags: feedback?(clokep) → feedback-
*** Original post on bio 2066 at 2013-07-25 10:40:39 UTC ***

Comment on attachment 8354405 [details] [diff] [review] (bio-attmnt 2636)
WIP

>diff --git a/chat/components/public/imIAccount.idl b/chat/components/public/imIAccount.idl
>+[scriptable, uuid(3fc279d9-b280-4f4a-babd-b4e328e65a01)]
>+interface prplIChatRoom: nsISupports {
>+  readonly attribute prplIAccount account;
>+  readonly attribute AUTF8String name;
>+  readonly attribute AUTF8String topic;
>+  readonly attribute prplIChatRoomFieldValues chatRoomFieldValues;
>+};
>+
>+[scriptable, uuid(d3c13bde-b852-44a8-a0dc-c781366cf110)]
>+interface prplIRequestChatRoomsCallback: nsISupports {
>+  void receiveChatRooms([array, size_is(aCount)] in prplIChatRoom aRooms, in unsigned long aCount);
>+};
I still find this slightly weird and would like Florian's opinion on this.

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>--- a/chat/protocols/irc/irc.js
>+++ b/chat/protocols/irc/irc.js
>@@ -1251,18 +1251,25 @@ ircAccount.prototype = {
>     // clients. Not doing it used to cause user confusion.
>     if (this.channelPrefixes.indexOf(channel[0]) == -1)
>       channel = "#" + channel;
>     let params = [channel];
>     let key = aComponents.getValue("password");
>     if (key)
>       params.push(key);
>     let defaultName = key ? channel + " " + key : channel;
>-    this._chatRoomFieldsList[this.normalize(channel)] =
>-      this.getChatRoomDefaultFieldValues(defaultName);
>+    let fields = this.getChatRoomDefaultFieldValues(defaultName);
>+    this._chatRoomFieldsList[this.normalize(channel)] = fields;
>+    let convAlreadyExists = this.hasConversation(channel);
>+    // Create the conversation tab first.
>+    let conv = this.getConversation(channel);
I know why you added this, but this really doesn't seem to be the right place to make this change. We'll definitely need to have a larger discussion (with Florian) about what wewant to do here.

>+    // Set _chatRoomFields on the new ircChannel if it didn't already exist to
>+    // prevent the rejoin message from being shown.
>+    if (!convAlreadyExists)
>+      conv._chatRoomFields = fields;
If we are making this change, we'll use a different mechanism here.

>     // Send the join command, but don't log the channel key.
>     this.sendMessage("JOIN", params,
>                      "JOIN " + channel + (key ? " <key not logged>" : ""));
>   },

>@@ -1271,16 +1278,29 @@ ircAccount.prototype = {
>+  _channelList: [],
>+  _listCallbacks: [],
These need to be set in the constructor too, right?

>+  requestChatRooms: function(aCallback) {
>+    this._listCallbacks.push(aCallback);
>+    this._initiateLIST();
>+  },
>+  _initiateLIST: function() {
I don't like this function name.

>+    if (this._LISTInitiated)
Nor this variable name.

>+      return;
>+    this._LISTInitiated = true;
>+    this.sendMessage("LIST");
>+  },

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>@@ -780,21 +781,34 @@ var ircBase = {
>     "322": function(aMessage) { // RPL_LIST
>       // <channel> <# visible> :<topic>
>-      // TODO parse this for # users & topic.
>+      this._channelList.push(aMessage);
You should be parsing the message here, not just saving it.

>       return serverMessage(this, aMessage);
>     },
>     "323": function(aMessage) { // RPL_LISTEND
>       // :End of LIST
>+      let parsedChannelList = this._channelList.map(function(aMessage) {
>+        let name = aMessage.params[1];
>+        let numParticipants = aMessage.params[2];
>+        let topic = aMessage.params[3];
>+        // Some servers return the channel's modes before the topic. Omit this.
>+        topic = topic.replace(/^\[\+.*\] /, "");
>+        return new ChatRoom(name, topic, this, this.getChatRoomDefaultFieldValues(name));
>+      }.bind(this));
Map takes a second parameter: the object to use as this.

>+      for (let callback of this._listCallbacks)
>+        callback.receiveChatRooms(parsedChannelList, parsedChannelList.length);
>+      this._listCallbacks = [];
>+      this._channelList = [];
>+      this._LISTInitiated = false;
You should define this in the prototype and delete it instead of setting it to false.
Comment on attachment 8354405 [details] [diff] [review]
WIP

*** Original change on bio 2066 attmnt 2636 at 2013-07-25 11:54:49 UTC ***

// Create the conversation tab first.

I agree with clokep that any change like this should be handled elsewhere. Changing the current API is not trivial and should also take care of bug 954270 (bio 837) (and the issues discussed there) if it is done. This doesn't mean you can't do it... but in a separate bug please :P

An alternative for now might be to CSS animate the listitem on click, so it's obvious something is happening. A simple option would be to "slide" it rightwards out of the window (use padding-left or something).

>>     "322": function(aMessage) { // RPL_LIST
>>       // <channel> <# visible> :<topic>
>>-      // TODO parse this for # users & topic.
>>+      this._channelList.push(aMessage);
>You should be parsing the message here, not just saving it.

Absolutely. And return results in batches, then return what's left on ENDLIST.

>if (aTopic == "account-connected")

Please don't request the chatroom list whenever an account connects. Do it when the first awesometab is opened? (This will also make a neat test of the dynamic updating of results in an open tab ;)

Also the accounts should cache their chatroomlist results and figure out themselves if they need to update it when the list is requested.

Have you resolved the issue of the LIST call freezing the UI?
Attachment #8354405 - Flags: feedback?(aleth) → feedback-
Attached patch Patch for api and backend (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2648 at 2013-07-27 20:08:00 UTC ***

This should be better, I hope.
I've made the serverName and infoText labels in newtab-items have the same priority for cropping. I'd like your opinions on this.
This patch doesn't include any protocol changes.
Attachment #8354417 - Flags: review?(florian)
Attachment #8354417 - Flags: review?(benediktp)
Attachment #8354417 - Flags: review?(aleth)
Comment on attachment 8354405 [details] [diff] [review]
WIP

*** Original change on bio 2066 attmnt 2636 at 2013-07-27 20:08:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354405 - Attachment is obsolete: true
Attachment #8354405 - Flags: feedback?(benediktp)
Attached file Implementation of the APIs in (obsolete) —
*** Original post on bio 2066 as attmnt 2649 at 2013-07-27 20:09:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
*** Original post on bio 2066 as attmnt 2650 at 2013-07-27 20:12:00 UTC ***

Sorry for the previous attachment, hit submit too soon by mistake.
Attachment #8354419 - Flags: review?(aleth)
Attachment #8354419 - Flags: review?(clokep)
Comment on attachment 8354418 [details]
Implementation of the APIs in

*** Original change on bio 2066 attmnt 2649 at 2013-07-27 20:12:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354418 - Attachment is obsolete: true
Attached patch Fix some bugs in LIST parsing (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2651 at 2013-07-27 20:18:00 UTC ***

Sorry for a new patch so soon, but I found some obvious bugs.
Attachment #8354420 - Flags: review?(aleth)
Attachment #8354420 - Flags: review?(clokep)
Comment on attachment 8354419 [details] [diff] [review]
Parse LIST data in IRC-js protocol as per changes in previous patch.

*** Original change on bio 2066 attmnt 2650 at 2013-07-27 20:18:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354419 - Attachment is obsolete: true
Attachment #8354419 - Flags: review?(clokep)
Attachment #8354419 - Flags: review?(aleth)
Attached patch Fix another obvious bug. (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2652 at 2013-07-27 20:21:00 UTC ***

Sorry again. I'm sleepy ;)
Attachment #8354421 - Flags: review?(aleth)
Attachment #8354421 - Flags: review?(clokep)
Comment on attachment 8354420 [details] [diff] [review]
Fix some bugs in LIST parsing

*** Original change on bio 2066 attmnt 2651 at 2013-07-27 20:21:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354420 - Attachment is obsolete: true
Attachment #8354420 - Flags: review?(clokep)
Attachment #8354420 - Flags: review?(aleth)
Comment on attachment 8354421 [details] [diff] [review]
Fix another obvious bug.

*** Original change on bio 2066 attmnt 2652 at 2013-07-28 04:47:36 UTC ***

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>@@ -761,16 +761,21 @@ function ircAccount(aProtocol, aImAccoun
>   this._requestedNickname = this._nickname;
> 
>   // For more information, see where these are defined in the prototype below.
>   this.trackQueue = [];
>   this.pendingIsOnQueue = [];
>   this.whoisInformation = {};
>   this._chatRoomFieldsList = {};
>   this._caps = [];
>+
>+  this._channelList = [];
>+  this._chatRoomCallbacks = [];
>+  this._gettingChannelList = false;
>+  this._lastChannelListRefresh = null;
Why are these last two being set in the constructor?

>@@ -896,16 +901,35 @@ ircAccount.prototype = {
>+  _channelList: [],
>+  _chatRoomCallbacks: [],
>+  _gettingChannelList: false,
We generally use "pending" for situations like this, I'd prefer the name "_pendingList" with a good comment describing what it does.

>+  _lastChannelListRefresh: null,
>+  _kChannelListRefreshInterval: 5 * 60 * 1000, // 5 minutes.
This shouldn't be an account property.

>+  requestChatRooms: function(aCallback) {
Comments about what this is doing, please.

>@@ -780,21 +781,41 @@ var ircBase = {
>     "322": function(aMessage) { // RPL_LIST
>       // <channel> <# visible> :<topic>
>-      // TODO parse this for # users & topic.
>+      let name = aMessage.params[1];
>+      let numParticipants = aMessage.params[2];
This is unused.

>+      let topic = aMessage.params[3];
>+      // Some servers return the channel's modes before the topic. Omit this.
>+      topic = topic.replace(/^\[\+.*\] /, "");
"some servers"? What servers? How do you know this?

>+      this._channelList.push(new ChatRoom(
>+        name, topic, this, this.getChatRoomDefaultFieldValues(name)));
ChatRoom? That seems like a poor object name for what this is (which I think is a potential conversation).

>     "323": function(aMessage) { // RPL_LISTEND
>       // :End of LIST
>+      let numUnreceivedChannels = this._channelList.length % 50;
>+      for (let callback of this._chatRoomCallbacks) {
>+        if (numUnreceivedChannels) {
Reverse the if and for here, there's no reason to check a conditional n times if it'll be the same for each. (In fact you can probably return early if this is false.)

This is lookinga lot better, I don't love it yet though.
Attachment #8354421 - Flags: review?(clokep) → review-
*** Original post on bio 2066 at 2013-07-28 05:31:50 UTC ***

> >+  _lastChannelListRefresh: null,
> >+  _kChannelListRefreshInterval: 5 * 60 * 1000, // 5 minutes.
> This shouldn't be an account property.

Ok, do you mean it should be defined globally?

> >+      let numParticipants = aMessage.params[2];
> This is unused.

Ah, yeah. I'll probably make prplIChatRoom use this and display the value somehow in the list items rather than remove it here.

> >+      let topic = aMessage.params[3];
> >+      // Some servers return the channel's modes before the topic. Omit this.
> >+      topic = topic.replace(/^\[\+.*\] /, "");
> "some servers"? What servers? How do you know this?

Moznet for example does this. Freenode doesn't. I know this by testing /list on them. I'll add to the comment if you'd like.

> >+      this._channelList.push(new ChatRoom(
> >+        name, topic, this, this.getChatRoomDefaultFieldValues(name)));
> ChatRoom? That seems like a poor object name for what this is (which I think is
> a potential conversation).

Er, it's a prplIChatRoom instance, and will be used to build a PossibleChat in the stats service - it needn't be used as a potential conversation by other consumers (though I wouldn't have an answer if you asked what else it would be used for :S).

> >     "323": function(aMessage) { // RPL_LISTEND
> >       // :End of LIST
> >+      let numUnreceivedChannels = this._channelList.length % 50;
> >+      for (let callback of this._chatRoomCallbacks) {
> >+        if (numUnreceivedChannels) {
> Reverse the if and for here, there's no reason to check a conditional n times
> if it'll be the same for each. (In fact you can probably return early if this
> is false.)

I can't return early because I'll need to call allChatRoomsReceived on all the callbacks. Either I need two loops (one for the unreceived channels and one for calling allChatRoomsReceived) or I'll get rid of allChatRoomsReceived (it's currently not used but I felt something like that was needed for a good API).

Another possibility is something like:

let fn = numUnreceivedChannels ? function(aCallback) {
aCallback.receiveChatRooms(...);
aCallback.allChatRoomsReceived(...);
} : function(aCallback) aCallback.allChatRoomsReceived(...);

for (let callback of this._chatRoomCallbacks)
  fn(callback);

Let me know what you think about this (the name "fn" will be changed) :).

> This is lookinga lot better, I don't love it yet though.

Thanks for the review!
Comment on attachment 8354421 [details] [diff] [review]
Fix another obvious bug.

*** Original change on bio 2066 attmnt 2652 at 2013-07-28 15:21:53 UTC ***

>+    let now = (new Date()).getTime();

You can use Date.now() for this.

>+    if (!this._lastChannelListRefresh)
>+      this._lastChannelListRefresh = now;

Maybe you can choose a better default value for this._lastChannelListRefresh that makes this unneccessary?

Something like _currentListDate may be a better name.

>     "322": function(aMessage) { // RPL_LIST
>       // <channel> <# visible> :<topic>
>-      // TODO parse this for # users & topic.
>+      let name = aMessage.params[1];
>+      let numParticipants = aMessage.params[2];

Please actually return this value so it can be used for sorting.

>+      let topic = aMessage.params[3];
>+      // Some servers return the channel's modes before the topic. Omit this.
>+      topic = topic.replace(/^\[\+.*\] /, "");
>+      this._channelList.push(new ChatRoom(
>+        name, topic, this, this.getChatRoomDefaultFieldValues(name)));

Do you really need to include the account object here? Won't this generate a XPConnect wrapper for each list entry?

>+      // Give callbacks batches of 50 channels.

Can we make this a const somewhere?
My guess is you might want this to be bigger as e.g. on freenode 80k/50=huge number of batches -- have you
investigated performance?

>+      if (this._channelList.length % 50 == 0) {
>+        for (let callback of this._chatRoomCallbacks)
>+          callback.receiveChatRooms(this._channelList.slice(-50), this, 50);
>+      }

Move this._channelList.slice(-50) outside the loop.
Is it more performant to use a separate counter and then reset it every time a batch is sent?

>     "323": function(aMessage) { // RPL_LISTEND
>       // :End of LIST
>+      let numUnreceivedChannels = this._channelList.length % 50;

numUnreceivedChannels is a really confusing name. These entries have been received.

>+        callback.allChatRoomsReceived(this, this._channelList.length);

Can we simplify the API by doing without this and instead return a boolean with receiveChatRooms indicating whether this is the last batch?
Attachment #8354421 - Flags: review?(aleth) → review-
Comment on attachment 8354417 [details] [diff] [review]
Patch for api and backend

*** Original change on bio 2066 attmnt 2648 at 2013-07-28 16:09:23 UTC ***

>+  // Maps an account id to another Map of chat names to PossibleChats.
>+  _chatsByAccIdAndName: new Map(),

Both the comment and the variable name are not clear. I had to read the code to understand what was going on.

>+  _addChat: function(aChat, aAccId) {
>+    if (!this._chatsByAccIdAndName.has(aAccId))
You can use let xxx = this._chatsByAccIdAndName.get(aAccId); if (!xxx) here, then you don't have to get the value again in the next if clause.

>+    else if (aTopic == "account-connected") {
>+      let account = aSubject;
>+      if (!account.canJoinChat)
>+        return;
>+      account.prplAccount.requestChatRooms(this._requestChatRoomsCallback);

I thought we weren't going to do this? ;)

Shouldn't existing entries for an account be removed when the account is disconnected?

>+  this._statusType = this.account.connected ?
>+    Ci.imIStatusInfo.STATUS_AVAILABLE : Ci.imIStatusInfo.STATUS_OFFLINE;

I don't think this status makes sense for possible chats. If the account is not connected the entry should not be there.
This is different from existing conversations, which may continue to exist even when the account is offline.

We've discussed the design of the MUC entries in the newtab on IRC.
Attachment #8354417 - Flags: review?(aleth) → review-
*** Original post on bio 2066 at 2013-07-28 17:31:30 UTC ***

(In reply to comment #10)
> > >+  _lastChannelListRefresh: null,
> > >+  _kChannelListRefreshInterval: 5 * 60 * 1000, // 5 minutes.
> > This shouldn't be an account property.
> Ok, do you mean it should be defined globally?
Yes.

> > >+      let topic = aMessage.params[3];
> > >+      // Some servers return the channel's modes before the topic. Omit this.
> > >+      topic = topic.replace(/^\[\+.*\] /, "");
> > "some servers"? What servers? How do you know this?
> 
> Moznet for example does this. Freenode doesn't. I know this by testing /list on
> them. I'll add to the comment if you'd like.
This was meant as "add a comment telling what servers do this" (and not which networks, but which servers).

> > >+      this._channelList.push(new ChatRoom(
> > >+        name, topic, this, this.getChatRoomDefaultFieldValues(name)));
> > ChatRoom? That seems like a poor object name for what this is (which I think is
> > a potential conversation).
> 
> Er, it's a prplIChatRoom instance, and will be used to build a PossibleChat in
> the stats service - it needn't be used as a potential conversation by other
> consumers (though I wouldn't have an answer if you asked what else it would be
> used for :S).
OK, but "new ChatRoom" sounds like you're creating a new MUC. that's not what you're doing at all, hence I think this is a poor interface and object name. (How is this different than prplIConvChat?) (And that's not a question of how it is actually different, I know that! When I ask questions like this, I mean "If I'm a new developer trying to figure this out, why am I supposed to know that prplIConvChat is the actual conversation and prplIChatRoom represents some room that exists on a server somewhere?"

> > >     "323": function(aMessage) { // RPL_LISTEND
> > >       // :End of LIST
> > >+      let numUnreceivedChannels = this._channelList.length % 50;
> > >+      for (let callback of this._chatRoomCallbacks) {
> > >+        if (numUnreceivedChannels) {
> > Reverse the if and for here, there's no reason to check a conditional n times
> > if it'll be the same for each. (In fact you can probably return early if this
> > is false.)
> 
> I can't return early because I'll need to call allChatRoomsReceived on all the
> callbacks. Either I need two loops (one for the unreceived channels and one for
> calling allChatRoomsReceived) or I'll get rid of allChatRoomsReceived (it's
> currently not used but I felt something like that was needed for a good API).
OK. I think this entire API is pretty confusing.
*** Original post on bio 2066 as attmnt 2671 at 2013-08-01 22:20:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354440 - Flags: review?(florian)
Attachment #8354440 - Flags: review?(benediktp)
Attachment #8354440 - Flags: review?(aleth)
Comment on attachment 8354417 [details] [diff] [review]
Patch for api and backend

*** Original change on bio 2066 attmnt 2648 at 2013-08-01 22:20:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354417 - Attachment is obsolete: true
Attachment #8354417 - Flags: review?(florian)
Attachment #8354417 - Flags: review?(benediktp)
Attached patch Patch for IRC's LIST (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2672 at 2013-08-01 22:35:00 UTC ***

I know there are a couple review points I haven't addressed, I will ask about them on IRC. I hope the rest of the patch is ok now.
Attachment #8354441 - Flags: review?(aleth)
Attachment #8354441 - Flags: review?(clokep)
Comment on attachment 8354421 [details] [diff] [review]
Fix another obvious bug.

*** Original change on bio 2066 attmnt 2652 at 2013-08-01 22:35:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354421 - Attachment is obsolete: true
*** Original post on bio 2066 as attmnt 2673 at 2013-08-01 22:43:00 UTC ***

I've made it show the participant count before the topic (e.g. "(30) Ask about Instantbird..."). I'll improve this if needed (which it probably is) after discussing on IRC.
Attachment #8354442 - Flags: review?(florian)
Attachment #8354442 - Flags: review?(benediktp)
Attachment #8354442 - Flags: review?(aleth)
Comment on attachment 8354440 [details] [diff] [review]
Patch for api, backend, and UI v2

*** Original change on bio 2066 attmnt 2671 at 2013-08-01 22:43:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354440 - Attachment is obsolete: true
Attachment #8354440 - Flags: review?(florian)
Attachment #8354440 - Flags: review?(benediktp)
Attachment #8354440 - Flags: review?(aleth)
Comment on attachment 8354441 [details] [diff] [review]
Patch for IRC's LIST

*** Original change on bio 2066 attmnt 2672 at 2013-08-02 21:56:26 UTC ***

>+var kChannelListRefreshInterval = 5 * 60 * 1000; // 5 minutes.

const! Similarly for kChannelsPerBatch.

>+  this._chatRoomCallbacks = [];

Adjust name to the changed interface name.

>+    let callbackIsNew = this._chatRoomCallbacks.indexOf(aCallback) == -1;

Make this._chatRoomCallbacks a Set.

>+    if (!this._lastChannelListRefresh)
>+      this._lastChannelListRefresh = now;

This is confusing because it means you set this._lastChannelListRefresh in two places. You don't need this if the default value for this._lastChannelListRefresh is 0.

>+    else if (now - this._lastChannelListRefresh < this.kChannelListRefreshInterval ||

Surely not this.kChannelListRefreshInterval now you've moved it outside the object?

>+      // If it's a new callback, receive the channels we already have.

Umm, you mean "pass to the callback". This is the sending end, not the receiving end. 

>+      // Some networks (e.g. Moznet) include the channel's modes before the topic.

Clokep asked you to find out which IRC server software does this, e.g. look on the server tab for moznet on connection to find out what they are running. Do you have other examples than Moznet?
Attachment #8354441 - Flags: review?(aleth) → review-
Comment on attachment 8354442 [details] [diff] [review]
Patch for api, backend, and UI v2.1

*** Original change on bio 2066 attmnt 2673 at 2013-08-02 22:08:37 UTC ***

>+  requestChatRooms: function(aCallback) { throw Cr.NS_ERROR_NOT_IMPLEMENTED; },

Should this be requestChatRoomInfos now? I'm not sure.

>+    // If a chat is already added, we remove it and re-add to refresh.
>+    else if (chatList.has(aRoomInfo.name)) {

What if two accounts have channels of the same name?

>+    // We request chat lists from accounts when adding new observers.
>+    let enumerator = Services.accounts.getAccounts();
>+    while (enumerator.hasMoreElements()) {
>+      let acc = enumerator.getNext();
>+      if (acc.connected && acc.canJoinChat) {
>+        try {
>+          acc.prplAccount.requestChatRooms(this._requestChatRoomsCallback);

You are relying here on your duplicate check when adding the returned chats. This means that chats that are removed from LIST never get removed from the service. I'm not sure the duplicate check for everything returned isn't quite slow either.

Instead how about
1) adding something to the API that tells us whether there is actually new data available (or will be fetched if we requestChatRooms).
2) If there is new data, remove all the existing entries for that account. If not, don't call requestChatRooms ;)

>+  this._statusType = Ci.imIStatusInfo.STATUS_AVAILABLE;

Why are we setting this? We don't need to display a status indicator for PossibleChats.

>-        Conversations.focusConversation(item.target.createConversation());
>+        let conv = item.target.createConversation();
>+        // If it's a chat, it won't return the conversation. So don't focus it.
>+        if (conv)
>+          Conversations.focusConversation(conv);

I guess this will need thinking about in the future, as you already mentioned.

>-        Conversations.focusConversation(this.target.createConversation());
>+        let conv = this.target.createConversation();
>+        // If it's a chat, it won't return the conversation. So don't focus it.
>+        if (conv)
>+          Conversations.focusConversation(conv);

This looks like duplicated code to me.
Attachment #8354442 - Flags: review?(aleth) → review-
Comment on attachment 8354441 [details] [diff] [review]
Patch for IRC's LIST

*** Original change on bio 2066 attmnt 2672 at 2013-08-03 01:39:07 UTC ***

I agree with aleth's comments, but here are more...

>+  // Channels are stored as prplIRoomInfo.
>+  _channelList: [],
>+  _chatRoomCallbacks: [],
>+  // If true, we have sent the LIST request and are waiting for replies.
>+  _pendingList: false,
>+  _lastChannelListRefresh: null,
>+  // Called by consumers that want a list of available channels, which are
>+  // provided through the callback (prplIRequestChatRoomsCallback instance).
>+  requestChatRooms: function(aCallback) {
>+    let callbackIsNew = this._chatRoomCallbacks.indexOf(aCallback) == -1;
>+    if (callbackIsNew)
>+      this._chatRoomCallbacks.push(aCallback);
Do callbacks ever get removed? What if I want to receive them from an extension? (This is why I suggested using observers btw...)
Attachment #8354441 - Flags: review?(clokep) → review-
Attached patch Patch for API and UI, v3 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2681 at 2013-08-04 01:36:00 UTC ***


> >+    // If a chat is already added, we remove it and re-add to refresh.
> >+    else if (chatList.has(aRoomInfo.name)) {
> 
> What if two accounts have channels of the same name?

chatList is specific to the account for id passed as a parameter.

> >+    // We request chat lists from accounts when adding new observers.
> >+    let enumerator = Services.accounts.getAccounts();
> >+    while (enumerator.hasMoreElements()) {
> >+      let acc = enumerator.getNext();
> >+      if (acc.connected && acc.canJoinChat) {
> >+        try {
> >+          acc.prplAccount.requestChatRooms(this._requestChatRoomsCallback);
> 
> You are relying here on your duplicate check when adding the returned chats.
> This means that chats that are removed from LIST never get removed from the
> service. I'm not sure the duplicate check for everything returned isn't quite
> slow either.

They're removed before requesting again now.

> Instead how about
> 1) adding something to the API that tells us whether there is actually new data
> available (or will be fetched if we requestChatRooms).
> 2) If there is new data, remove all the existing entries for that account. If
> not, don't call requestChatRooms ;)

I don't know if there's a way to tell whether new data will be fetched, before actually fetching it. Since I don't intend any protocol implementation to maintain or refresh chat room lists without being requested by a consumer, I don't see a way to let them (consumers) know that there's new data. I'm willing to discuss this on IRC.

> >-        Conversations.focusConversation(this.target.createConversation());
> >+        let conv = this.target.createConversation();
> >+        // If it's a chat, it won't return the conversation. So don't focus it.
> >+        if (conv)
> >+          Conversations.focusConversation(conv);
> 
> This looks like duplicated code to me.

I couldn't think of a good way to de-duplicate this. :(
Attachment #8354450 - Flags: review?(florian)
Attachment #8354450 - Flags: review?(benediktp)
Attachment #8354450 - Flags: review?(aleth)
Comment on attachment 8354442 [details] [diff] [review]
Patch for api, backend, and UI v2.1

*** Original change on bio 2066 attmnt 2673 at 2013-08-04 01:36:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354442 - Attachment is obsolete: true
Attachment #8354442 - Flags: review?(florian)
Attachment #8354442 - Flags: review?(benediktp)
Attached patch Patch for IRC's LIST v3 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2682 at 2013-08-04 01:47:00 UTC ***

Callbacks are removed at LISTEND now. I've included the api to remove callbacks though in case something wants to remove it after requesting but before LISTEND.
Attachment #8354451 - Flags: review?(aleth)
Attachment #8354451 - Flags: review?(clokep)
Comment on attachment 8354441 [details] [diff] [review]
Patch for IRC's LIST

*** Original change on bio 2066 attmnt 2672 at 2013-08-04 01:47:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354441 - Attachment is obsolete: true
*** Original post on bio 2066 at 2013-08-04 13:45:06 UTC ***

(In reply to comment #20)
> > Instead how about
> > 1) adding something to the API that tells us whether there is actually new data
> > available (or will be fetched if we requestChatRooms).
> > 2) If there is new data, remove all the existing entries for that account. If
> > not, don't call requestChatRooms ;)
> 
> I don't know if there's a way to tell whether new data will be fetched, before
> actually fetching it. Since I don't intend any protocol implementation to
> maintain or refresh chat room lists without being requested by a consumer, I
> don't see a way to let them (consumers) know that there's new data. I'm willing
> to discuss this on IRC.
 
You could have an boolean attribute (getter) on the account that tells you whether the account thinks its cached roomlist info is stale or not, i.e. whether it will just return cached data when you request the info or whether it would query the server. Basically just move the corresponding if clause in the current code to an attribute.
Summary: New conversation tab should display chat rooms → New conversation tab should suggest chat rooms
Comment on attachment 8354451 [details] [diff] [review]
Patch for IRC's LIST v3

*** Original change on bio 2066 attmnt 2682 at 2013-08-05 12:32:08 UTC ***

Not sure if this patch is still current (does it incorporate the interface improvements discussed on IRC?)

>+const kListRefreshInterval = 60 * 60 * 1000; // 1 hour.

IMHO this could be even higher, maybe ask clokep?

Also I dimly recall hearing some servers kick you for requesting LIST, do we know if this is still true and if so, which?

>+  requestRoomInfo: function(aCallback) {
>+    let callbackIsNew = !this._roomInfoCallbacks.has(aCallback);
>+    if (callbackIsNew)
>+      this._roomInfoCallbacks.add(aCallback);
>+    // We don't send the LIST request if it hasn't been longer than
>+    // kListRefreshInterval since the last one.
>+    let now = Date.now();
>+    if (now - this._lastListTime < kListRefreshInterval || this._pendingList) {

The if clause I was referring to in my previous comment.

>+      // Pass channel info we already have to the callback if it's new.
>+      if (callbackIsNew)
>+        aCallback.receiveRoomInfo(this._channelList, this, this._channelList.length);
>+      return;

The callback is never removed if we are just returning cached data.
Attachment #8354451 - Flags: review?(aleth) → review-
*** Original post on bio 2066 at 2013-08-05 12:50:49 UTC ***

(In reply to comment #23)
> Comment on attachment 8354451 [details] [diff] [review] (bio-attmnt 2682) [details]
> Patch for IRC's LIST v3
> 
> Not sure if this patch is still current (does it incorporate the interface
> improvements discussed on IRC?)
> 
> >+const kListRefreshInterval = 60 * 60 * 1000; // 1 hour.
> 
> IMHO this could be even higher, maybe ask clokep?
> 
> Also I dimly recall hearing some servers kick you for requesting LIST, do we
> know if this is still true and if so, which?

Maybe we should even have a safeguard that prevents using LIST on an account in the future if a connection was ever closed by the server shortly after requesting the data? Is there a way that we can link a closed connection to sending the request, e.g. a particular message or error code or something (forgive me my ignorance regarding details of the inner workings of IRC, please;)?
Comment on attachment 8354451 [details] [diff] [review]
Patch for IRC's LIST v3

*** Original change on bio 2066 attmnt 2682 at 2013-08-12 13:23:40 UTC ***

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>+const kListRefreshInterval = 60 * 60 * 1000; // 1 hour.
This seems way too often. It'd be nice to be able to like subscribe to these changes. We might need to look into this more. I'd suggest 12 hours or once a day for now.

>@@ -896,16 +900,45 @@ ircAccount.prototype = {
>+  requestRoomInfo: function(aCallback) {
>+    let callbackIsNew = !this._roomInfoCallbacks.has(aCallback);
>+    if (callbackIsNew)
>+      this._roomInfoCallbacks.add(aCallback);
>+    // We don't send the LIST request if it hasn't been longer than
>+    // kListRefreshInterval since the last one.
>+    let now = Date.now();
>+    if (now - this._lastListTime < kListRefreshInterval || this._pendingList) {
It doesn't look like now needs to be stored into a variable.

>+      // Pass channel info we already have to the callback if it's new.
>+      if (callbackIsNew)
>+        aCallback.receiveRoomInfo(this._channelList, this, this._channelList.length);
>+      return;
>+    }
I really dislike this API with caring whether the callback was just added or not.

>+    this._channelList = [];
>+    this._pendingList = true;
>+    this.sendMessage("LIST");
Doesn't this need to update this._lastListTime here to Date.now()?

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>+const kChannelsPerBatch = 250; // Callbacks receive this many channels per batch.
What is a "batch"? Maybe it should be kChannelsPerNotification or something.

>@@ -780,21 +783,42 @@ var ircBase = {
>     "322": function(aMessage) { // RPL_LIST
>       // <channel> <# visible> :<topic>
>-      // TODO parse this for # users & topic.
>+      let name = aMessage.params[1];
>+      let numParticipants = aMessage.params[2];
>+      let topic = aMessage.params[3];
>+      // Some servers (e.g. Unreal) include the channel's modes before the topic.
>+      // Omit this.
>+      topic = topic.replace(/^\[\+.*\] /, "");
Nit: Add a blank line here.

>+      this._channelList.push(new RoomInfo(name, topic, numParticipants,
>+        this.imAccount.id, this.getChatRoomDefaultFieldValues(name)));
If you're in the chat room, you'll probably want to get the stored field values from it, since those might contain a password. (Perhaps we should store this information in a better way internally...)

>     "323": function(aMessage) { // RPL_LISTEND
[...]
>+      this._roomInfoCallbacks.clear();
Clear all the callbacks? So they need to re-register? I'm a bit confused about this API still...can you give a text description of what consumers are expected to do?

>+      this._pendingList = false;
Please use "delete this._pendingList;".
Attachment #8354451 - Flags: review?(clokep) → review-
Comment on attachment 8354450 [details] [diff] [review]
Patch for API and UI, v3

*** Original change on bio 2066 attmnt 2681 at 2013-08-16 13:58:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354450 - Flags: review?(florian) → review-
*** Original post on bio 2066 at 2013-08-16 13:58:23 UTC ***

Comment on attachment 8354450 [details] [diff] [review] (bio-attmnt 2681)
Patch for API and UI, v3

>+[scriptable, uuid(3fc279d9-b280-4f4a-babd-b4e328e65a01)]
>+interface prplIRoomInfo: nsISupports {
>+  readonly attribute AUTF8String accountId;
>+  readonly attribute AUTF8String name;
>+  readonly attribute AUTF8String topic;
>+  readonly attribute unsigned long numParticipants;

participantCount

>+  readonly attribute prplIChatRoomFieldValues chatRoomFieldValues;
>+};
>+
>+/*
>+ * This interface should be implemented by callbacks passed to an account's
>+ * requestRoomInfo function.
>+ */
>+
>+[scriptable, function, uuid(d3c13bde-b852-44a8-a0dc-c781366cf110)]
>+interface prplIRoomInfoCallback: nsISupports {
>+  /* aRooms is an array containing a batch of prplIRoomInfo. This will be called
>+   * multiple times as batches of chat rooms are received. The number of rooms
>+   * in each batch is left for the prplIAccount implementation to decide.
>+   */
>+  void receiveRoomInfo([array, size_is(aCount)] in prplIRoomInfo aRooms,
>+                        in prplIAccount aAccount,
>+                        in unsigned long aCount);

Fix the indent here.

Also, receiveRoomInfo doesn't sounds right. Maybe receive*d*RoomInfo? Other suggestions: roomInfoReceived, onDataAvailable, onRoomInfoAvailable, onRoomInfoReceived?



>+  /* Request information on available chat room,

chat room*s*.

>+     Callbacks will continue to receive
>+   * (possibly duplicate) room info until they are removed using removeRoomInfoCallback.

It makes no sense to have an API do that.

>+  void requestRoomInfo(in prplIRoomInfoCallback aCallback);

Maybe just "get" instead of "request"?

>+  void removeRoomInfoCallback(in prplIRoomInfoCallback aCallback);

Would be nice if we could avoid needing this.

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

>+function RoomInfo(aName, aTopic, aNumParticipants, aAccountId, aChatRoomFieldValues) {
>+  this.name = aName;
>+  this.topic = aTopic;
>+  this.numParticipants = aNumParticipants;
>+  this.accountId = aAccountId;
>+  this.chatRoomFieldValues = aChatRoomFieldValues;
>+}
>+RoomInfo.prototype = ClassInfo("prplIRoomInfo", "RoomInfo object");

Is this helper actually helpful? It doesn't seem to do much.


>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js

>   getFilteredConvs: function(aFilterStr, aFilteredConvsCount) {
>     // Clone this._contacts to ensure it doesn't get modified
>     // when we insert ui conversations.
>     let filteredConvs = this._contacts.slice(0);
>+    filteredConvs = filteredConvs.concat(this._chats);

Doesn't the .concat here make the .slice(0) above unnecessary?


>     let existingConvs = Services.conversations.getUIConversations().map(
>                           function(uiConv) new ExistingConversation(uiConv));
>     for (let existingConv of existingConvs) {
>       let pos = this._getPositionToInsert(existingConv, filteredConvs);
>       filteredConvs.splice(pos, 0, existingConv);
>-      // Remove the contact to prevent a duplicate.
>-      let contact = existingConv.uiConv.contact;
>-      if (contact && this._contactsById.has(contact.id)) {
>-        filteredConvs.splice(
>-          filteredConvs.indexOf(this._contactsById.get(contact.id)), 1);
>+      // Remove any duplicate contact or chat.
>+      let uiConv = existingConv.uiConv;
>+      if (existingConv.isChat) {
>+        let chatList = this._chatsByAccountIdAndName.get(uiConv.account.id);
>+        if (chatList) {
>+          let chat = chatList.get(uiConv.name);
>+          if (chat)
>+            filteredConvs.splice(filteredConvs.indexOf(chat), 1);
>+        }
>+      }
>+      else {
>+        let contact = uiConv.contact;
>+        if (contact && this._contactsById.has(contact.id)) {
>+          filteredConvs.splice(
>+            filteredConvs.indexOf(this._contactsById.get(contact.id)), 1);
>+        }
>       }
>     }
>     if (aFilterStr) {
>       aFilterStr = aFilterStr.toLowerCase();
>       filteredConvs = filteredConvs.filter(function(c) {
>         return c.lowerCaseName.startsWith(aFilterStr) ||
>           c.lowerCaseName.split(/\s+/).some(function(s) s.startsWith(aFilterStr));
>       });
>     }
>     if (aFilteredConvsCount)
>       aFilteredConvsCount.value = filteredConvs.length;
>     return filteredConvs;
>   },
> 
>   addObserver: function(aObserver) {
>-    if (this._observers.indexOf(aObserver) == -1)
>-      this._observers.push(aObserver);
>+    if (this._observers.indexOf(aObserver) != -1)
>+      return;
>+    this._observers.push(aObserver);
>+    // We request chat lists from accounts when adding new observers.
>+    for (let id of this._accountIdsToRequestRoomInfo) {
>+      this._removeChatsForAccount(id);
>+      let acc = Services.accounts.getAccountById(id);
>+      if (acc.connected && acc.canJoinChat) {
>+        try {

Would it simplify things to do here:
  let accId = acc.id;

>+          acc.prplAccount.requestRoomInfo(function(aRoomInfo, aPrplAccount) {
>+            let id = aPrplAccount.imAccount.id;
>+            for (let room of aRoomInfo)
>+              this._addChat(room, id);

And to use here acc.id instead of id. Or room.accountId?

Or _addChat could do room.accountId itself, and the above 3 lines would be simplified to aRoomInfo.forEach(this._addChat, this);

>+            Cu.reportError("received batch");

Please cleanup the next patch :-).


>+        // We force-refresh the chat room list every hour.
>+        setTimeout(function() {
>+          this._accountIdsToRequestRoomInfo.add(id);
>+        }.bind(this), 60 * 60 * 1000);

Shouldn't this timer be canceled if the list of observers becomes empty? I don't want my Instantbird to fetch room lists every hours all night long.


>+function PossibleChat(aRoomInfo) {
>+  this._accountId = aRoomInfo.accountId;
>+  this._displayName = aRoomInfo.name;
>+  this._isChat = true;
>+  this._statusType = null;
>+  this._statusText = "(" + aRoomInfo.numParticipants + ") " +
>+    (aRoomInfo.topic || _instantbird("noTopic"));
>+  this._infoText = this.account.normalizedName;
>+  this._buddyIconFilename = "";
>+  this._chatRoomFieldValues = aRoomInfo.chatRoomFieldValues;

Values initialized to constants (_isChat, _statusType, _buddyIconFilename) should be in the prototype.


>-  // A stats-service-contact-updated notification is fired when a contact has
>-  // been updated and observers need to refresh.
>+  // A stats-service-updated notification is fired when data has been updated
>+  // and observers need to refresh.

observers don't need to 'refresh'. The UI may need to.

>diff --git a/instantbird/content/newtab.xml b/instantbird/content/newtab.xml

>-        Conversations.focusConversation(item.target.createConversation());
>+        let conv = item.target.createConversation();
>+        // If it's a chat, it won't return the conversation. So don't focus it.
>+        if (conv)
>+          Conversations.focusConversation(conv);

Isn't this a bug? If so it would be nice for the comment to include the bug number.


>+          let source = aTarget.source;
>+          this.setAttribute("source", source);
>+          if (source != "chat") {
>+            if (!("Status" in window))
>+              Components.utils.import("resource:///modules/imStatusUtils.jsm");
>+            this.setAttribute("status", Status.toAttribute(aTarget.statusType));
>+          }
>+          this.setAttribute("chat", aTarget.isChat);

Why do you need the chat attribute if you already set the source attribute?
If you have a real reason to need the chat attribute, wouldn't it be cleaner to only set it when it's true?

This way the CSS could just do [chat] instead of [chat="true"].

>     <handlers>
>       <handler event="mouseup">
>       <![CDATA[
>         if (event.button == 2) // Don't handle right clicks.
>           return;
>         let newtab = document.getBindingParent(this);
>         if (!("Conversations" in window))
>           Components.utils.import("resource:///modules/imWindows.jsm");
>-        Conversations.focusConversation(this.target.createConversation());
>+        let conv = this.target.createConversation();
>+        // If it's a chat, it won't return the conversation. So don't focus it.
>+        if (conv)
>+          Conversations.focusConversation(conv);

This smells a bit like code duplication.
Attached patch Patch for IRC's LIST v4 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2727 at 2013-08-17 03:06:00 UTC ***

I haven't addressed a couple of clokep's comments because I want to discuss some things (for example, I don't think ChannelsPerNotification is a good name since there aren't really any notifications).
Attachment #8354496 - Flags: review?(aleth)
Attachment #8354496 - Flags: review?(clokep)
Comment on attachment 8354451 [details] [diff] [review]
Patch for IRC's LIST v3

*** Original change on bio 2066 attmnt 2682 at 2013-08-17 03:06:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354451 - Attachment is obsolete: true
Attached patch Patch for API and UI v4 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2728 at 2013-08-17 03:12:00 UTC ***

>Isn't this a bug? If so it would be nice for the comment to include the bug
>number.

I don't think there's a specific bug on this.

>Maybe just "get" instead of "request"?

I used request to be consistent with requestBuddyInfo.

>Is this helper actually helpful? It doesn't seem to do much.

I did this because I didn't like doing the ClassInfo stuff in the protocol implementation. I can remove it if you'd like.
Attachment #8354497 - Flags: review?(florian)
Comment on attachment 8354450 [details] [diff] [review]
Patch for API and UI, v3

*** Original change on bio 2066 attmnt 2681 at 2013-08-17 03:12:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354450 - Attachment is obsolete: true
Attachment #8354450 - Flags: review?(benediktp)
Attachment #8354450 - Flags: review?(aleth)
*** Original post on bio 2066 at 2013-08-17 10:07:09 UTC ***

(In reply to comment #28)
> Created attachment 8354497 [details] [diff] [review] (bio-attmnt 2728) [details]
> Patch for API and UI v4
> 
> >Isn't this a bug? If so it would be nice for the comment to include the bug
> >number.
> 
> I don't think there's a specific bug on this.

So it looks like the Join Chat dialog focuses chatrooms only if they were already joined (either in an existing tab or on hold), but doesn't do it if it's a channel where the user wasn't yet.

I couldn't find an exact bug for this. Two bugs are loosely related (worth reading, but not including the number in your new code): bug 954747 (bio 1315) and bug 954270 (bio 837).
*** Original post on bio 2066 at 2013-08-17 12:14:02 UTC ***

For a followup: Decide what to do with iSUPPORT ELIST and SAFELIST http://www.irc.org/tech_docs/005.html
Comment on attachment 8354496 [details] [diff] [review]
Patch for IRC's LIST v4

*** Original change on bio 2066 attmnt 2727 at 2013-08-17 15:59:31 UTC ***

>+  get roomInfoReady() Date.now() - this._lastListTime > kListRefreshInterval,

How about isRoomInfoReady to make it clear it is a boolean? Personally I would prefer flipping it to isRoomInfoOld or isRoomInfoStale as that corresponds better to how it is used.

>+    let callbackIsNew = !this._roomInfoCallbacks.has(aCallback);

Now you've cleaned this up, you only use this once.

>+    // We don't send the LIST request if it hasn't been longer than
>+    // kListRefreshInterval since the last one, or a request is already pending,
>+    // So we pass channel info we already have to the callback if it's new.

How about instead returning early if it's not new? I don't understand why if it's not new we need to do anything at all.

>+  removeRoomInfoCallback: function(aCallback) {
>+    this._roomInfoCallbacks.remove(aCallback);
>+  },

I thought we were getting rid of this?

>+const kChannelsPerBatch = 250; // Callbacks receive this many channels per batch.

You mean "at most this many channels on each call".

>     "323": function(aMessage) { // RPL_LISTEND
>       // :End of LIST
>+      let numRemainingChannels = this._channelList.length % kChannelsPerBatch;
>+      if (numRemainingChannels) {
>+        let remainingChannels = this._channelList.slice(-numRemainingChannels);
>+        for (let callback of this._roomInfoCallbacks)
>+          callback.onRoomInfoAvailable(remainingChannels, this, numRemainingChannels);
>+      }

Shouldn't onRoomInfoAvailable have a boolean parameter telling the callback that this is the final set of channels (and that the whole set was received without problems)? If not, there should be a comment in the API description explaining that there is no way to know if one has received all the channels.
Attachment #8354496 - Flags: review?(aleth) → review-
Comment on attachment 8354497 [details] [diff] [review]
Patch for API and UI v4

*** Original change on bio 2066 attmnt 2728 at 2013-08-17 16:33:35 UTC ***

>+/*
>+ * This interface should be implemented by callbacks passed to an account's
>+ * requestRoomInfo function.
>+ */

Somewhat misleading as it's possible to just pass a function as a callback, due to the function attribute.

>+
>+[scriptable, function, uuid(d3c13bde-b852-44a8-a0dc-c781366cf110)]
>+interface prplIRoomInfoCallback: nsISupports {
>+  /* aRooms is an array containing a batch of prplIRoomInfo. This will be called
>+   * multiple times as batches of chat rooms are received. The number of rooms
>+   * in each batch is left for the prplIAccount implementation to decide.
>+   */
>+  void onRoomInfoAvailable([array, size_is(aCount)] in prplIRoomInfo aRooms,
>+                           in prplIAccount aAccount,
>+                           in unsigned long aCount);
>+};

>+  /* Request information on available chat rooms, which will be returned via the
>+   * onRoomInfoReceived function of the callback.

You changed the name I think.

Ditto for numParticipants.

>+  _accountIdsToRequestRoomInfo: new Set(),

This needs a comment.

>+  _addChat: function(aRoomInfo) {
>+    let accId = aRoomInfo.accountId;

Maybe we could afford to spell this out (accountId) ;)

>+    // We request chat lists from accounts when adding new observers.
>+    for (let id of this._accountIdsToRequestRoomInfo) {
>+      let acc = Services.accounts.getAccountById(id);
>+      if (acc.connected && acc.canJoinChat && acc.prplAccount.roomInfoReady) {

If roomInfoReady is false, when do we call requestRoomInfo?

>+        try {
>+          acc.prplAccount.requestRoomInfo(function(aRoomInfo, aPrplAccount) {
>+            aRoomInfo.forEach(this._addChat, this);
>+            this._notifyObservers("updated");
>+          }.bind(this));
>+        } catch(e) {
>+          if (e.result != Cr.NS_ERROR_NOT_IMPLEMENTED)
>+            Cu.reportError(e);
>+          continue;
>+        }
>+        // After an hour, we remove chat room data for the account and add it
>+        // to the queue to request room info next time an observer is added.

Please explain why we are doing this? (also in the comment)

>+        setTimeout(function() {
>+          this._removeChatsForAccount(id);
>+          this._accountIdsToRequestRoomInfo.add(id);
>+        }.bind(this), 60 * 60 * 1000);
>+      }
>+    }
>+    this._accountIdsToRequestRoomInfo.clear();
>   },

>+    else if (aTopic == "account-connected")
>+      this._accountIdsToRequestRoomInfo.add(aSubject.id);
>+    else if (aTopic == "account-disconnected") {
>+      let id = aSubject.id;
>+      this._removeChatsForAccount(id);
>+      this._accountIdsToRequestRoomInfo.delete(id);
>+    }

Please think through (also in the other patch) the situation where an account is disconnected and/or reconnected. This will interrupt LIST for example. How is this handled (the existing callbacks, possibly rerequesting the info, etc)?
Attachment #8354497 - Flags: review?(florian) → review-
Attached patch Patch for IRC's LIST v5 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2733 at 2013-08-18 15:35:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354502 - Flags: review?(aleth)
Attachment #8354502 - Flags: review?(clokep)
Comment on attachment 8354496 [details] [diff] [review]
Patch for IRC's LIST v4

*** Original change on bio 2066 attmnt 2727 at 2013-08-18 15:35:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354496 - Attachment is obsolete: true
Attachment #8354496 - Flags: review?(clokep)
Attached patch Patch for API and UI v5 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2734 at 2013-08-18 15:37:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354503 - Flags: review?(aleth)
Attachment #8354503 - Flags: review?(florian)
Comment on attachment 8354497 [details] [diff] [review]
Patch for API and UI v4

*** Original change on bio 2066 attmnt 2728 at 2013-08-18 15:37:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354497 - Attachment is obsolete: true
Comment on attachment 8354502 [details] [diff] [review]
Patch for IRC's LIST v5

*** Original change on bio 2066 attmnt 2733 at 2013-08-18 15:42:28 UTC ***

>+    // We don't send the LIST request if it hasn't been longer than
>+    // kListRefreshInterval since the last one, or a request is already pending,
>+    // So we pass channel info we already have to the callback if it's new.

You can drop the "if it's new" now.

Looks good otherwise!
Attachment #8354502 - Flags: review?(aleth) → review-
Comment on attachment 8354503 [details] [diff] [review]
Patch for API and UI v5

*** Original change on bio 2066 attmnt 2734 at 2013-08-18 16:13:27 UTC ***

>   addObserver: function(aObserver) {
>-    if (this._observers.indexOf(aObserver) == -1)
>-      this._observers.push(aObserver);
>+    if (this._observers.indexOf(aObserver) != -1)
>+      return;
>+    this._observers.push(aObserver);
>+    // We request chat lists from accounts when adding new observers.
>+    for (let id of this._accountIdsToRequestRoomInfo) {
>+      let acc = Services.accounts.getAccountById(id);
>+      if (acc.connected && acc.canJoinChat && acc.prplAccount.isRoomInfoStale) {
>+        // Discard any chat room data we already have.
>+        this._removeChatsForAccount(id);
>+        try {
>+          acc.prplAccount.requestRoomInfo(function(aRoomInfo, aPrplAccount) {
>+            aRoomInfo.forEach(this._addChat, this);
>+            this._notifyObservers("updated");
>+          }.bind(this));
>+        } catch(e) {
>+          if (e.result != Cr.NS_ERROR_NOT_IMPLEMENTED)
>+            Cu.reportError(e);
>+          continue;
>+        }
>+        // After an hour, we add the account to the queue to request room info
>+        // next time an observer is added, to ensure chat rooms are up to date.
>+        setTimeout(function() {
>+          this._accountIdsToRequestRoomInfo.add(id);
>+        }.bind(this), 60 * 60 * 1000);
>+      }
>+    }
>+    this._accountIdsToRequestRoomInfo.clear();
>   },

This doesn't really make sense to me. We let the protocol decide how often to request room info, so we shouldn't need a second timeout here. Instead, why don't we, for all connected accounts with chat, request room info if (acc.prplAccount.isRoomInfoStale || we haven't already got info for that account).

I haven't closely reviewed the layout of the MUC listitems, as it will be easy to improve those in followups.
Attachment #8354503 - Flags: review?(aleth) → review-
Attached patch Patch for API and UI v6 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2735 at 2013-08-18 18:51:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354504 - Flags: review?(aleth)
Attachment #8354504 - Flags: review?(florian)
Comment on attachment 8354503 [details] [diff] [review]
Patch for API and UI v5

*** Original change on bio 2066 attmnt 2734 at 2013-08-18 18:51:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354503 - Attachment is obsolete: true
Attachment #8354503 - Flags: review?(florian)
Attached patch Patch for IRC's LIST v6 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2736 at 2013-08-18 18:52:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354505 - Flags: review?(aleth)
Attachment #8354505 - Flags: review?(clokep)
Comment on attachment 8354502 [details] [diff] [review]
Patch for IRC's LIST v5

*** Original change on bio 2066 attmnt 2733 at 2013-08-18 18:52:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354502 - Attachment is obsolete: true
Attachment #8354502 - Flags: review?(clokep)
Comment on attachment 8354505 [details] [diff] [review]
Patch for IRC's LIST v6

*** Original change on bio 2066 attmnt 2736 at 2013-08-18 18:57:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354505 - Flags: review?(aleth) → review+
Comment on attachment 8354504 [details] [diff] [review]
Patch for API and UI v6

*** Original change on bio 2066 attmnt 2735 at 2013-08-18 19:11:16 UTC ***

I think this is ready to try in nightlies :)

We should consider removing some of the data cached in the service after some timeout, but I think these kind of memory improvements can be done in a followup, especially as the addition of the ranking/database may change how and where we store things.
Attachment #8354504 - Flags: review?(aleth) → review+
Comment on attachment 8354505 [details] [diff] [review]
Patch for IRC's LIST v6

*** Original change on bio 2066 attmnt 2736 at 2013-08-20 11:55:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354505 - Flags: review?(clokep) → review-
*** Original post on bio 2066 at 2013-08-20 11:55:02 UTC ***

Comment on attachment 8354505 [details] [diff] [review] (bio-attmnt 2736)
Patch for IRC's LIST v6

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>@@ -742,16 +742,17 @@ ircAccountBuddy.prototype = {
>+const kListRefreshInterval = 12 * 60 * 60 * 1000; // 12 hours.
I'd prefer this at the top of the file.

>@@ -761,16 +762,19 @@ function ircAccount(aProtocol, aImAccoun
>+  this._channelList = [];
It looks like this is always reset before used below, so we probably don't need to set this in the constructor.

>@@ -896,16 +900,58 @@ ircAccount.prototype = {
>                 aSetter || this._currentServerName);
>       }
>       this.getConversation(this._currentServerName)
>           .writeMessage(this._currentServerName, msg, {system: true});
>     }
>     return true;
>   },
> 
>+  // Channels are stored as prplIRoomInfo.
>+  _channelList: [],
>+  _roomInfoCallbacks: new Set(),
>+  // If true, we have sent the LIST request and are waiting for replies.
>+  _pendingList: false,
>+  // Callbacks receive at most this many channels per call.
>+  kChannelsPerBatch: 250,
This isn't really a constant, it seems weird to have a property starting with k.

>+  _lastListTime: 0,
>+  get isRoomInfoStale() Date.now() - this._lastListTime > kListRefreshInterval,

>+  // Called by consumers that want a list of available channels, which are
>+  // provided through the callback (prplIRoomInfoCallback instance).
>+  requestRoomInfo: function(aCallback) {
>+    if (this._roomInfoCallbacks.has(aCallback)) // Callback is not new.
>+      return;
>+    // We don't send the LIST request if it hasn't been longer than
>+    // kListRefreshInterval since the last one, or a request is already pending,
>+    // So we pass channel info we already have to the callback.
This sentence has a lot of double negatives in it, I think it should be "Only send the LIST request if it has been longer than kListRefreshInterval since the last one and a request is not currently pending." The last line here I don't understand.

>+    if (!this.isRoomInfoStale || this._pendingList)
>+      aCallback.onRoomInfoAvailable(this._channelList, this, this._channelList.length);
>+    else {
>+      this._channelList = [];
>+      this._pendingList = true;
>+      this._lastListTime = Date.now();
>+      this.sendMessage("LIST");
>+    }

Nit: Put an empty line here.
>+    if (this._pendingList)
>+      this._roomInfoCallbacks.add(aCallback);
>+  },
>+  // Pass room info for any remaining channels to callbacks and clean up.
>+  _sendRemainingRoomInfo: function() {
>+    let remainingChannelCount = this._channelList.length % this.kChannelsPerBatch;
>+    if (remainingChannelCount) {
>+      let remainingChannels = this._channelList.slice(-remainingChannelCount);
>+      for (let callback of this._roomInfoCallbacks) {
>+        callback.onRoomInfoAvailable(remainingChannels, this,
>+                                     remainingChannelCount, true);
>+      }
>+    }
>+    this._roomInfoCallbacks.clear();
>+    delete this._pendingList;
>+  },

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>@@ -780,21 +781,38 @@ var ircBase = {
>      */
>     "321": function(aMessage) { // RPL_LISTSTART
>       // Channel :Users Name
>       // Obsolete. Not used.
>       return true;
>     },
>     "322": function(aMessage) { // RPL_LIST
>       // <channel> <# visible> :<topic>
>-      // TODO parse this for # users & topic.
>+      let name = aMessage.params[1];
>+      let participantCount = aMessage.params[2];
>+      let topic = aMessage.params[3];
>+      // Some servers (e.g. Unreal) include the channel's modes before the topic.
>+      // Omit this.
>+      topic = topic.replace(/^\[\+.*\] /, "");
>+
>+      this._channelList.push(new RoomInfo(name, topic, participantCount,
>+        this.imAccount.id, this.getChatRoomDefaultFieldValues(name)));
>+      // Give callbacks a batch of channels of length kChannelsPerBatch.
>+      if (this._channelList.length % this.kChannelsPerBatch == 0) {
>+        let channelBatch = this._channelList.slice(-this.kChannelsPerBatch);
>+        for (let callback of this._roomInfoCallbacks) {
>+          callback.onRoomInfoAvailable(channelBatch, this, this.kChannelsPerBatch,
>+                                       false);
>+        }
>+      }
>       return serverMessage(this, aMessage);
Do we still want to report this as a message (and have the command for this)?
*** Original post on bio 2066 at 2013-08-20 11:59:45 UTC ***

(In reply to comment #40)
> Do we still want to report this as a message (and have the command for this)?

Iirc the argument for keeping it was that e.g. gervase was using it do get a dump of LIST output he could then filter himself in non-obvious ways. If we're sure the awesometab search/filtering is good enough to remove the need for this we can remove /list. I don't think it's quite there yet myself.
Attached patch Patch for API and UI v7 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2741 at 2013-08-20 14:48:00 UTC ***

Changed noMoreRooms to aNoMoreRooms.
Changed order of aCount and aNoMoreRooms parameters.
Attachment #8354510 - Flags: review?(aleth)
Attachment #8354510 - Flags: review?(florian)
Comment on attachment 8354504 [details] [diff] [review]
Patch for API and UI v6

*** Original change on bio 2066 attmnt 2735 at 2013-08-20 14:48:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354504 - Attachment is obsolete: true
Attachment #8354504 - Flags: review?(florian)
Attached patch Patch for IRC's LIST v7 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2742 at 2013-08-20 14:51:00 UTC ***

Addressed review comments and made necessary changes to accommodate API change above.
Attachment #8354511 - Flags: review?(clokep)
Comment on attachment 8354505 [details] [diff] [review]
Patch for IRC's LIST v6

*** Original change on bio 2066 attmnt 2736 at 2013-08-20 14:51:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354505 - Attachment is obsolete: true
Comment on attachment 8354510 [details] [diff] [review]
Patch for API and UI v7

*** Original change on bio 2066 attmnt 2741 at 2013-08-20 14:52:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354510 - Flags: review?(aleth) → review+
Comment on attachment 8354511 [details] [diff] [review]
Patch for IRC's LIST v7

*** Original change on bio 2066 attmnt 2742 at 2013-08-20 15:16:11 UTC ***

Let's give this a try in the nightlies and see if we get kicked for flooding.

Please file a follow-up to:
 - Check the ISUPPORT code about whether or not LIST can be done.
Attachment #8354511 - Flags: review?(clokep) → review+
Comment on attachment 8354510 [details] [diff] [review]
Patch for API and UI v7

*** Original change on bio 2066 attmnt 2741 at 2013-08-30 21:41:31 UTC ***

>+   * aNoMoreRooms will be true when aRooms is the last batch.

I dislike this name. I would prefer if we could avoid the negation in the name. Maybe aFinished or aCompleted?


>   prplIChatRoomFieldValues getChatRoomDefaultFieldValues([optional] in AUTF8String aDefaultChatName);
>+  /* Request information on available chat rooms, which will be returned via the
>+   * onRoomInfoAvailable function of the callback.

Drop the "the onRoomInfoAvailable function of " from this sentence; it adds no information.


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

>+  removeRoomInfoCallback: function(aCallback) { throw Cr.NS_ERROR_NOT_IMPLEMENTED; },

Is this a line from a previous patch that you forgot to remove?



>   getFilteredConvs: function(aFilterStr, aFilteredConvsCount) {
>     // Clone this._contacts to ensure it doesn't get modified
>     // when we insert ui conversations.

You likely want to rephrase this comment a bit.


>   addObserver: function(aObserver) {

>+      if (acc.connected && acc.canJoinChat && (acc.prplAccount.isRoomInfoStale ||
>+          !this._chatsByAccountIdAndName.has(id))) {

For some reason I think it would make more sense to check _chatsByAccountIdAndName before isRoomInfoStale.


>+        // Discard any chat room data we already have.
>+        this._removeChatsForAccount(id);
>+        try {
>+          acc.prplAccount.requestRoomInfo(function(aRoomInfo, aPrplAccount) {
>+            aRoomInfo.forEach(this._addChat, this);

Why is the third parameter (the boolean saying if we are done) never used?


>+function PossibleChat(aRoomInfo) {

>+  this._statusText = "(" + aRoomInfo.participantCount + ") " +
>+    (aRoomInfo.topic || _instantbird("noTopic"));

I think we will want to find something better to display the participant count, but this will do for a first version.

>+  this._infoText = this.account.normalizedName;

This likely isn't great either.

> function ExistingConversation(aUIConv) {
>   this._id = aUIConv.target.id;
>   this._displayName = aUIConv.title;
>   this._isChat = aUIConv.isChat;
>   if (aUIConv.isChat) {
>     this._statusText = aUIConv.topic || _instantbird("noTopic");
>-    this._statusType = aUIConv.account.connected && !aUIConv.left ?
>-      Ci.imIStatusInfo.STATUS_AVAILABLE : Ci.imIStatusInfo.STATUS_OFFLINE;
>+    this._statusType = null;

How is null handled? Shouldn't it rather be STATUS_UNKNOWN?


>+      <method name="startConversation">
>+        <parameter name="aNewtabItem"/>
>+        <body>
>+        <![CDATA[
>+          let conv = aNewtabItem.target.createConversation();
>+          // If it's a chat, it won't return the conversation. So don't focus it.

I think you meant "If it's a *new* chat". If the user is already in a chat (ie. we are just switching to the tab) you can focus the conversation, right? :-)



>       <method name="build">

>+          if (source != "chat") {
>+            if (!("Status" in window))
>+              Components.utils.import("resource:///modules/imStatusUtils.jsm");
>+            this.setAttribute("status", Status.toAttribute(aTarget.statusType));
>+          }
>+          if (aTarget.isChat)

The difference between the (source != "chat") and the (aTarget.isChat) tests isn't obvious.


>+/* void requestRoomInfo (in prplIRoomInfoCallback aCallback); */
>+NS_IMETHODIMP purpleAccount::RequestRoomInfo(prplIRoomInfoCallback *aCallback)
>+{
>+    return NS_ERROR_NOT_IMPLEMENTED;

Fix the indent here please.

>+}
>+
>+/* readonly attribute boolean isRoomInfoStale; */
>+NS_IMETHODIMP purpleAccount::GetIsRoomInfoStale(bool *aIsRoomInfoStale)
>+{
>+    return NS_ERROR_NOT_IMPLEMENTED;

And here.
Attachment #8354510 - Flags: review?(florian) → review-
Attached patch Patch for API and UI v8 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2808 at 2013-08-30 22:18:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354578 - Flags: review?(florian)
Comment on attachment 8354510 [details] [diff] [review]
Patch for API and UI v7

*** Original change on bio 2066 attmnt 2741 at 2013-08-30 22:18:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354510 - Attachment is obsolete: true
Attached patch Patch for IRC's LIST v8 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2809 at 2013-08-30 22:36:00 UTC ***

Fix indentation
Attachment #8354579 - Flags: review?(florian)
Comment on attachment 8354511 [details] [diff] [review]
Patch for IRC's LIST v7

*** Original change on bio 2066 attmnt 2742 at 2013-08-30 22:36:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354511 - Attachment is obsolete: true
Comment on attachment 8354578 [details] [diff] [review]
Patch for API and UI v8

*** Original change on bio 2066 attmnt 2808 at 2013-08-30 22:42:24 UTC ***

>+/*
>+ * Contains information about a chat room and the fields required to join it.

Drop the "Contains" word.

>+ */
>+

No empty line between the comment and the interface if the comment is only about one interface.
We have an empty line when the comment is about a section (several interfaces).

>+/*
>+ * Interface for callbacks passed to an account's requestRoomInfo function.
>+ */
>+

Same here. And drop "Interface for". And "callbacks" should be singluar.



>+  requestRoomInfo: function(aCallback) { throw Cr.NS_ERROR_NOT_IMPLEMENTED; },
>+  get isRoomInfoStale() false,

Here you are returning false by default...

>+/* readonly attribute boolean isRoomInfoStale; */
>+NS_IMETHODIMP purpleAccount::GetIsRoomInfoStale(bool *aIsRoomInfoStale)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;

... and throwing NOT_IMPLEMENTED here. This is slightly inconsistent (remember how these kind of inconsistencies were fun for the user icon path? ;)), I wonder if the C++ method should return false, or if jsProtoHelper should throw.
Attachment #8354578 - Flags: review?(florian) → review-
Comment on attachment 8354579 [details] [diff] [review]
Patch for IRC's LIST v8

*** Original change on bio 2066 attmnt 2809 at 2013-08-30 22:49:04 UTC ***

The already has clokep and aleth's review, it doesn't need mine.
Attachment #8354579 - Flags: review?(florian)
Attached patch Patch for API and UI v9 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2811 at 2013-08-31 17:43:00 UTC ***

Ensure update notifications are fired at most once per second, address review comments.
Attachment #8354581 - Flags: review?(florian)
Comment on attachment 8354578 [details] [diff] [review]
Patch for API and UI v8

*** Original change on bio 2066 attmnt 2808 at 2013-08-31 17:43:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354578 - Attachment is obsolete: true
Blocks: 955582
*** Original post on bio 2066 at 2013-09-01 15:57:37 UTC ***

Do we really need it to be as staggered as once per second? How about once per 100ms?
Blocks: 955584
Attached patch Patch for API and UI v10 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2815 at 2013-09-03 16:52:00 UTC ***

- Change staggering to 100ms as suggested
- Fix bug where newtab-items that were built from a MUC would still show the MUC icon after rebuilding to a non-MUC.
Attachment #8354585 - Flags: review?(aleth)
Attachment #8354585 - Flags: review?(florian)
Comment on attachment 8354581 [details] [diff] [review]
Patch for API and UI v9

*** Original change on bio 2066 attmnt 2811 at 2013-09-03 16:52:38 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354581 - Attachment is obsolete: true
Attachment #8354581 - Flags: review?(florian)
*** Original post on bio 2066 at 2013-09-04 07:20:19 UTC ***

Comment on attachment 8354579 [details] [diff] [review] (bio-attmnt 2809)
Patch for IRC's LIST v8

When is the content of _channelList released? It looks like if the data is older than kListRefreshInterval, we won't ever use it again. Maybe we should also drop it sooner if the account is disconnected? (probably not immediately after the account is disconnected, as if the user is on a flaky connection we don't want to fetch the channel list over and over again; but maybe after an hour? or 20 minutes?) Maybe we should also release all this data when a memory pressure notification is fired.

Do we still need to batch by 250 channels now that the updates are throttled on the stats service?

Note: this isn't an r- comment; just things to think about.
Comment on attachment 8354585 [details] [diff] [review]
Patch for API and UI v10

*** Original change on bio 2066 attmnt 2815 at 2013-09-04 07:22:13 UTC ***

I reviewed only the interdiff between attachment 8354578 [details] [diff] [review] (bio-attmnt 2808) and this, and it looked good; thanks!

I haven't tried the patches in this bug on my local build at all. Maybe I should before we land this?
Attachment #8354585 - Flags: review?(florian) → review+
Attached patch Patch for API and UI v11 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2818 at 2013-09-04 18:50:00 UTC ***

Small change: add "account-disconnected" to kNotificationsToObserve list.
Attachment #8354588 - Flags: review?(florian)
Comment on attachment 8354585 [details] [diff] [review]
Patch for API and UI v10

*** Original change on bio 2066 attmnt 2815 at 2013-09-04 18:50:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354585 - Attachment is obsolete: true
Attachment #8354585 - Flags: review?(aleth)
Attached patch Patch for IRC's LIST v9 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2828 at 2013-09-05 17:56:00 UTC ***

Add pref to disable requestRoomInfo
Comment on attachment 8354579 [details] [diff] [review]
Patch for IRC's LIST v8

*** Original change on bio 2066 attmnt 2809 at 2013-09-05 17:56:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354579 - Attachment is obsolete: true
Comment on attachment 8354598 [details] [diff] [review]
Patch for IRC's LIST v9

*** Original change on bio 2066 attmnt 2828 at 2013-09-05 17:59:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354598 - Flags: review?(clokep)
Attached patch Patch for IRC's LIST v10 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2829 at 2013-09-05 18:17:00 UTC ***

Change name of pref to "chat.irc.allowList" and move the pref below the other IRC pref as discussed on IRC
Attachment #8354599 - Flags: review?(clokep)
Comment on attachment 8354598 [details] [diff] [review]
Patch for IRC's LIST v9

*** Original change on bio 2066 attmnt 2828 at 2013-09-05 18:17:32 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354598 - Attachment is obsolete: true
Attachment #8354598 - Flags: review?(clokep)
Attached patch Patch for IRC's LIST v11 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2830 at 2013-09-05 18:29:00 UTC ***

This patch should work, sorry.
Attachment #8354600 - Flags: review?(clokep)
Comment on attachment 8354599 [details] [diff] [review]
Patch for IRC's LIST v10

*** Original change on bio 2066 attmnt 2829 at 2013-09-05 18:29:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354599 - Attachment is obsolete: true
Attachment #8354599 - Flags: review?(clokep)
Comment on attachment 8354600 [details] [diff] [review]
Patch for IRC's LIST v11

*** Original change on bio 2066 attmnt 2830 at 2013-09-05 18:43:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354600 - Flags: review?(clokep) → review+
Attached patch Patch for API and UI v12 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2831 at 2013-09-05 18:51:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354601 - Flags: review?(florian)
Comment on attachment 8354588 [details] [diff] [review]
Patch for API and UI v11

*** Original change on bio 2066 attmnt 2818 at 2013-09-05 18:51:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354588 - Attachment is obsolete: true
Attachment #8354588 - Flags: review?(florian)
Attached patch Patch for API and UI v13 (obsolete) — Splinter Review
*** Original post on bio 2066 as attmnt 2832 at 2013-09-05 22:19:00 UTC ***

This changes the getFilteredConvs API to return an nsSimpleEnumerator, and implements the necessary changes in the newtab binding. This should improve performance drastically when the list of conversations is large.
Attachment #8354602 - Flags: review?(florian)
Comment on attachment 8354601 [details] [diff] [review]
Patch for API and UI v12

*** Original change on bio 2066 attmnt 2831 at 2013-09-05 22:19:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354601 - Attachment is obsolete: true
Attachment #8354601 - Flags: review?(florian)
*** Original post on bio 2066 as attmnt 2833 at 2013-09-05 23:47:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354603 - Flags: review?(florian)
Comment on attachment 8354602 [details] [diff] [review]
Patch for API and UI v13

*** Original change on bio 2066 attmnt 2832 at 2013-09-05 23:47:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354602 - Attachment is obsolete: true
Attachment #8354602 - Flags: review?(florian)
*** Original post on bio 2066 as attmnt 2834 at 2013-09-05 23:50:00 UTC ***

Pref off, and change channelsPerBatch to 25.
Attachment #8354604 - Flags: review?(clokep)
Comment on attachment 8354600 [details] [diff] [review]
Patch for IRC's LIST v11

*** Original change on bio 2066 attmnt 2830 at 2013-09-05 23:50:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354600 - Attachment is obsolete: true
Comment on attachment 8354604 [details] [diff] [review]
Patch for IRC's LIST v12

*** Original change on bio 2066 attmnt 2834 at 2013-09-05 23:51:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354604 - Flags: review?(clokep) → review+
*** Original post on bio 2066 at 2013-09-06 00:16:33 UTC ***

http://hg.instantbird.org/instantbird/rev/c52c3dd7677f
http://hg.instantbird.org/instantbird/rev/0bc4f51cc78b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Depends on: 955595
Depends on: 955597
Comment on attachment 8354603 [details] [diff] [review]
Patch for API and UI v14

*** Original change on bio 2066 attmnt 2833 at 2013-09-10 00:06:03 UTC ***

Forgot to put the r+ flag after checking in the patch... :-)
Attachment #8354603 - Flags: review?(florian) → review+
Blocks: 955013
Depends on: 955662
Blocks: 959365
You need to log in before you can comment on or make changes to this bug.