Currently, once we find the feature stanza for the MUC, we record the service [1] and use it to populate the join chat dialog (the "server" input box). It would be nice if we could do room discovery too and use it in the New Conversation tab. I _think_ that means implementing prplIAccount::requestRoomInfo(). [1]: https://dxr.mozilla.org/comm-central/rev/b7472e239aa2/chat/protocols/xmpp/xmpp.jsm#1502

Comment on attachment 8765616 [details] [diff] [review] v1 - protocol-independant changes Review of attachment 8765616 [details] [diff] [review]: ----------------------------------------------------------------- ::: im/components/ibConvStatsService.js @@ +695,5 @@ > }, > get statusText() { > let roomInfo = this.account.prplAccount.getRoomInfo(this.displayName); > + let text = ""; > + if (roomInfo.participantCount >= 0) That's not going to work as is, participantCount is an unsigned long.

Comment on attachment 8765614 [details] [diff] [review] v1 - Support Room Discovery Review of attachment 8765614 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1048,5 @@ > + }, > + get participantCount() { > + let jid = this._account._roomList.get(this.name); > + if (!this._account._mucs.has(jid)) > + return -1; unsigned long... @@ +1708,5 @@ > }); > }, > > + requestRoomInfo: function(aCallback) { > + this._roomList = new Map(); Add a mechanism to not request the list from the server again every time a new tab is opened, similar to https://dxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#968 @@ +1711,5 @@ > + requestRoomInfo: function(aCallback) { > + this._roomList = new Map(); > + this._roomInfoCallback = aCallback; > + > + // XEP-0045 (6.5): Querying for Room Items. 6.3, not room items, I think? @@ +1733,5 @@ > + // XEP-0059: Result Set Management. > + let set = query.getElement(["set"]); > + let last = set ? set.getElement(["last"]) : null; > + let isCompleted; > + if (last && items.length > 0) { Can last && items.length == 0 happen? @@ +1745,5 @@ > + else > + isCompleted = true; > + > + let rooms = items.map(item => { > + let name = item.attributes["name"]; Can it happen that this is undefined? e.g. 6.3 example 7

Comment on attachment 8765614 [details] [diff] [review] v1 - Support Room Discovery Review of attachment 8765614 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1048,5 @@ > + }, > + get participantCount() { > + let jid = this._account._roomList.get(this.name); > + if (!this._account._mucs.has(jid)) > + return -1; We probably want to add a constant for this (like what we did for typing notifications).

(In reply to aleth [:aleth] from comment #5) > 6.3, not room items, I think? Yes, that was wrong. > Can last && items.length == 0 happen? I added it to avoid infinite recursive calls (I'm not sure if the last result will contain last element as I didn't see that in the spec XEP-0059). BTW, I'll check this again today. > Can it happen that this is undefined? e.g. 6.3 example 7 Not sure, but I tested room discovery with a real server and returned jid and name, but I think this can happen.

Comment on attachment 8766331 [details] [diff] [review] v3 - protocol-independant changes Review of attachment 8766331 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/components/public/imIAccount.idl @@ +54,1 @@ > readonly attribute unsigned long participantCount; Can't you just make this a signed long? ;)

Comment on attachment 8766417 [details] [diff] [review] v2 - Support Room Discovery Review of attachment 8766417 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks much better, but there are some problems. There are important but slightly nonstandard servers. For example, on hipchat in response to the disco#info on the conf jid one receives (instead of?) a room list <iq xmlns="jabber:client" from="conf.hipchat.com" type="result" id="14164f12-9889-7f43-a549-9a1cd6859da4" to="42_041@chat.hipchat.com/Instantbird||proxy|pubproxy-b300.hipchat.com|5242"> <query xmlns="http://jabber.org/protocol/disco#items"> <item xmlns="http://jabber.org/protocol/disco#items" jid="42_ibdev@conf.hipchat.com" name="Ibdev"> <x xmlns="http://hipchat.com/protocol/muc#room"> <id xmlns="http://hipchat.com/protocol/muc#room"> 1875 </id> <name xmlns="http://hipchat.com/protocol/muc#room"> Ibdev </name> <topic xmlns="http://hipchat.com/protocol/muc#room"> Welcome! Send this link to coworkers who need accounts: https://www.hipchat.com/invite/42/321a1be58d2d31826c29d62270a?utm_campaign=company_room_link </topic> <privacy xmlns="http://hipchat.com/protocol/muc#room"> public </privacy> <owner xmlns="http://hipchat.com/protocol/muc#room"> 42_041@chat.hipchat.com </owner> <guest_url xmlns="http://hipchat.com/protocol/muc#room"/> <version xmlns="http://hipchat.com/protocol/muc#room"> YUBD453G </version> <num_participants xmlns="http://hipchat.com/protocol/muc#room"> 0 </num_participants> </x> </item> </query> </iq> The current code then does disco#info on the <item> and fails to parse the result properly. This looks so different that it can wait for a followup. Slack works well up to the point where the room list is received, but in response to disco#info on individual rooms you get service-unavailable. The current code handles this error but in a way that means the rooms which we know to exist never show up in the newtab. The code should make use of the info we do have. ::: chat/protocols/xmpp/xmpp.jsm @@ +1768,5 @@ > + > + // XEP-0045 (6.4): Querying for Room Information. > + let isLast = index == (items.length - 1); > + let iq = Stanza.iq("get", null, jid, > + Stanza.node("query", Stanza.NS.disco_info)); This is a nice idea, but if the server has many rooms, we'll be flooding the server with requests and potentially get disconnected or throttled. So unfortunately it won't work. So there are two options - Decide you don't care about participant count (and sometimes name) and just use the room list data as before (parsing the jid for the room name when not available). - Lazily request this info when getRoomInfo is called. But because getRoomInfo is called when a room needs to be displayed, it has to return data immediately and can't wait for a round trip to the server. So if you want to add this, you need to write some notification mechanism that sends some data immediately and follows up with more data if/when the results arrive from the server. This has to be done in a way that doesn't make scrolling in the newtab janky. Up to you if you think this effort is worth it just for the participant count. - Maybe there is a third option I don't know about, some XEP similar to entity capabilities for presence...

(In reply to aleth [:aleth] from comment #12) > So there are two options > - Decide you don't care about participant count (and sometimes name) and > just use the room list data as before (parsing the jid for the room name > when not available). > - Lazily request this info when getRoomInfo is called. But because > getRoomInfo is called when a room needs to be displayed, it has to return > data immediately and can't wait for a round trip to the server. So if you > want to add this, you need to write some notification mechanism that sends > some data immediately and follows up with more data if/when the results > arrive from the server. This has to be done in a way that doesn't make > scrolling in the newtab janky. Up to you if you think this effort is worth > it just for the participant count. > - Maybe there is a third option I don't know about, some XEP similar to > entity capabilities for presence... > This has to be done in a way that doesn't make scrolling in the newtab > janky. This needs to be tested well and I'm not sure about the behaviour, I think we can go with the first option and let the second one be an improvement.

(In reply to aleth [:aleth] from comment #12) > - Maybe there is a third option I don't know about, some XEP similar to > entity capabilities for presence... What about enqueuing the result of room list (jids) then request room info sequentially (when we get a response from room info, trigger the next request)?

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #14) > (In reply to aleth [:aleth] from comment #12) > > - Maybe there is a third option I don't know about, some XEP similar to > > entity capabilities for presence... > > What about enqueuing the result of room list (jids) then request room info > sequentially (when we get a response from room info, trigger the next > request)? To avoid blocking this bug, let's leave experimenting with detailed room infos for a followup.

(In reply to aleth [:aleth] from comment #15) > To avoid blocking this bug, let's leave experimenting with detailed room > infos for a followup. OK.

Comment on attachment 8766571 [details] [diff] [review] v4 - protocol-independant changes Review of attachment 8766571 [details] [diff] [review]: ----------------------------------------------------------------- Typo in the commit message - please fix before checkin

Comment on attachment 8767344 [details] [diff] [review] v3 - Support Room Discovery Review of attachment 8767344 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1082,5 @@ > // Contains the domain of MUC service which is obtained using service > // discovery. > _mucService: null, > > + // Maps room names to {jid}. room jid not {jid} @@ +1773,5 @@ > + let jid = item.attributes["jid"]; > + > + // We already have this muc. > + if (this._mucs.has(jid)) > + return; Not a good idea. The roomList should contain all mucs because you may leave the room before the next time the roomList is fetched from the server. @@ +1777,5 @@ > + return; > + > + let name = item.attributes["name"]; > + if (!name) > + name = jid; Don't use the full jid, just the first part that isn't the same for all rooms. That's more readable.

Comment on attachment 8767401 [details] [diff] [review] v4 - Support Room Discovery Review of attachment 8767401 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1767,5 @@ > + else > + this._pendingList = false; > + > + let items = query.getElements(["item"]); > + let test = this._parseJID("test@test2.com").node; ? @@ +1769,5 @@ > + > + let items = query.getElements(["item"]); > + let test = this._parseJID("test@test2.com").node; > + let rooms = items.map(item => { > + let jid = this._parseJID(item.attributes["jid"]); Now you're parsing it, you can null check the result just in case

Comment on attachment 8767519 [details] [diff] [review] v5 - Support Room Discovery Review of attachment 8767519 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: chat/protocols/xmpp/xmpp.jsm @@ +1768,5 @@ > + this._pendingList = false; > + > + let rooms = []; > + let items = query.getElements(["item"]); > + items.forEach(item => { You could shorten this to query.getElements(["item"]).forEach if you like

(In reply to aleth [:aleth] from comment #17) > Typo in the commit message - please fix before checkin Fixed https://hg.mozilla.org/comm-central/rev/6f19fc2df369 (In reply to aleth [:aleth] from comment #22) > You could shorten this to query.getElements(["item"]).forEach if you like Done before checkin https://hg.mozilla.org/comm-central/rev/28caf547300d