I will not be able to take a look at this before December so unassigning myself.

I left the UI part (conversation.xml) to determine the nick of received typing to a follow up bug as it uses node instead of resource in this case (node@domain/resource) The spec for private messages in MUC xep-0045 [1]. [1] http://xmpp.org/extensions/xep-0045.html#privatemessage

Comment on attachment 8617505 [details] [diff] [review] rev 1 - support private muc message Review of attachment 8617505 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for starting on this tricky bug! ::: chat/protocols/xmpp/xmpp.jsm @@ +351,5 @@ > supportChatStateNotifications: true, > _typingState: "active", > > + // helps UI determine the sender from the full jid. > + _privateMUC: false, I was a bit confused about the meaning of this at first. Maybe it should be called _isMucParticipant? It definitely needs a longer comment with examples of what the JIDs look like. @@ +355,5 @@ > + _privateMUC: false, > + get privateMUC() this._privateMUC, > + set privateMUC (aFlag) { > + this._privateMUC = aFlag; > + }, It doesn't look like you need a getter or setter here, the boolean is enough. @@ +395,5 @@ > return; > > + let notInRoom = (aError) => { > + // We do not want to notify the user for each character he/she writes that > + // he/she is not in the room. Why does this happen? We shouldn't be sending any stanzas that just result in errors. @@ +417,5 @@ > _targetResource: "", > get to() { > + if (!this._targetResource) > + return this.userName; > + return this._account.normalize(this.userName) + "/" + this._targetResource; Can we add a comment with some examples here? Why the normalize() call? I'm surprised the privateMUC flag doesn't appear here. @@ +429,5 @@ > + let muc = this._account._mucs.get(norm); > + let message; > + > + // XEP-004 (7.5): MUC private messages. > + if (muc && !muc.left && aError.condition == "item-not-found") { Should check _privateMUC too. @@ +431,5 @@ > + > + // XEP-004 (7.5): MUC private messages. > + if (muc && !muc.left && aError.condition == "item-not-found") { > + // If the recipient is not in the room. > + let resource = this._account._parseJID(from).resource; This is a bit confusing. Should there be a function essentially the same as _parseJID, but for MUC JIDs, so that this would be _parseMucJID(from).nick? @@ +1332,5 @@ > } > > + let convName = norm; > + if (this._mucs.has(norm)) > + convName = from; Needs a comment. @@ +1333,5 @@ > > + let convName = norm; > + if (this._mucs.has(norm)) > + convName = from; > + let conv = this.createConversation(convName); createConversation takes a normalizedName as a parameter. There's a tricky issue here, of which this is probably only the first example: currently normalize(MUC participant JID) strips the nick from the jid and so is no longer suitable as a normalizedName for the conversation: http://mxr.mozilla.org/comm-central/source/chat/components/public/prplIConversation.idl#34 Have you thought about how to deal with this? What should the normalizedNick of a MUC participant private chat be? I'm sorry to have to bring it up, as normalization issues are painful, but it's unavoidably a part of this bug. Also notice http://mxr.mozilla.org/comm-central/source/chat/components/public/prplIConversation.idl#110 which is called by the UI. @@ +1574,5 @@ > } > + let conv = this._conv.get(aNormalizedName); > + if (!conv) > + return undefined; > + let norm = this.normalize(aNormalizedName); This is very confusing, as generally it's assumed that normalizing a normalizedName has no effect.

(In reply to aleth [:aleth] from comment #4) > createConversation takes a normalizedName as a parameter. Correction: despite the parameter name in xmpp.jsm, I don't think this is actually true, see https://dxr.mozilla.org/comm-central/source/chat/components/public/imIAccount.idl#99 > Also notice > http://mxr.mozilla.org/comm-central/source/chat/components/public/ > prplIConversation.idl#110 > which is called by the UI. Note the XMPP part of that comment may well change as you fix this bug...

(In reply to aleth [:aleth] from comment #4) > Maybe it should be called _isMucParticipant? > > It definitely needs a longer comment with examples of what the JIDs look > like. yes, you are right. > Why does this happen? We shouldn't be sending any stanzas that just result > in errors. This happens because we are sending messages when typing status changes and if the user is not in the room, for every change the user will get an error message in conversation UI. OK, we can use left flag to avoid that before sending, but there are cases also need to be handled like the recipient is not in the room or the room does not exist or the recipient does not exist. > Can we add a comment with some examples here? Why the normalize() call? > > I'm surprised the privateMUC flag doesn't appear here. OK, we normalize here to avoid appending resource to it more than time like username(room@domain/nick) and if the target resource has nick. If we do not normalize username the result would be (room@doamin/nick/nick). When the room responds this causes username to be (room@domain/nick/nick) and we will be in infinite loop of appends for every message sent. I do not use privateMUC because I think these checks solves this issue for all cases. > Should check _privateMUC too. Yes, you are right. > This is a bit confusing. Should there be a function essentially the same as > _parseJID, but for MUC JIDs, so that this would be _parseMucJID(from).nick? I would be the same, but the order of result would be different, so I thought we can user _parseJID and reorder the result, so does this need a different name function? > There's a tricky issue here, of which this is probably only the first > example: currently normalize(MUC participant JID) strips the nick from the > jid and so is no longer suitable as a normalizedName for the conversation: > http://mxr.mozilla.org/comm-central/source/chat/components/public/ > prplIConversation.idl#34 > > Have you thought about how to deal with this? What should the normalizedNick > of a MUC participant private chat be? I think it could be only the nick. (In reply to aleth [:aleth] from comment #5) > Correction: despite the parameter name in xmpp.jsm, I don't think this is > actually true, see Can we change the parameter in xmpp.jsm to be aName instead of aNormalizedName?

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #6) > > Why does this happen? We shouldn't be sending any stanzas that just result > > in errors. > This happens because we are sending messages when typing status changes and > if the user is not in the room, for every change the user will get an error > message in conversation UI. > OK, we can use left flag to avoid that before sending, but there are cases > also need to be handled like the recipient is not in the room or the room > does not exist or the recipient does not exist. That's OK, I was just checking it doesn't happen all the time. > > Can we add a comment with some examples here? Why the normalize() call? > > > > I'm surprised the privateMUC flag doesn't appear here. > OK, we normalize here to avoid appending resource to it more than time like > username(room@domain/nick) and if the target resource has nick. If we do not > normalize username the result would be (room@doamin/nick/nick). > When the room responds this causes username to be (room@domain/nick/nick) > and we will be in infinite loop of appends for every message sent. > I do not use privateMUC because I think these checks solves this issue for > all cases. I understand that, but for a private message with a MUC participant, what is the expected value of targetResource? Unlike a contact, there can't be more than one resource at any given time for a MUC participant, as the resource is the nick. > > This is a bit confusing. Should there be a function essentially the same as > > _parseJID, but for MUC JIDs, so that this would be _parseMucJID(from).nick? > I would be the same, but the order of result would be different, so I > thought we can user _parseJID and reorder the result, so does this need a > different name function? If it improves readability of the code, adding an alias for _parseJID may be a good idea. > > There's a tricky issue here, of which this is probably only the first > > example: currently normalize(MUC participant JID) strips the nick from the > > jid and so is no longer suitable as a normalizedName for the conversation: > > http://mxr.mozilla.org/comm-central/source/chat/components/public/ > > prplIConversation.idl#34 > > Have you thought about how to deal with this? What should the normalizedNick > > of a MUC participant private chat be? > I think it could be only the nick. That won't work as normalizedNames are supposed to be unique (and "nick" in room1 and "nick" in room2 may not be the same person), otherwise e.g. the logs for the two will be mixed together. > (In reply to aleth [:aleth] from comment #5) > > Correction: despite the parameter name in xmpp.jsm, I don't think this is > > actually true, see > Can we change the parameter in xmpp.jsm to be aName instead of > aNormalizedName? Yes, it seems this should be changed, and createConversation and its callers should be checked to make sure it still works properly when passed an unnormalized name as a parameter.

Comment on attachment 8623519 [details] [diff] [review] rev 2 - support private muc message Review of attachment 8623519 [details] [diff] [review]: ----------------------------------------------------------------- this._conv is currently a NormalizedMap. I think that's going to cause problems as soon as there are converations with two different participants of the same MUC. Note if you decide to turn it into a normal Map, you have to to go through the existing code and make sure all operations on this._conv normalises the keys... ::: chat/protocols/xmpp/xmpp.jsm @@ +383,5 @@ > > + // Returns jid (room@domain/nick) if it is a private muc, and the name of > + // conversation otherwise. > + get normalizedName() > + this._isMucParticipant ? this.userName : this._account.normalize(this.name), You should still apply the other parts of normalize(), i.e. lowercase. @@ +399,5 @@ > + // be in form of room@domain/nick and if the nick could not be parsed, > + // return the jid. > + if (this._isMucParticipant) { > + if (!jid.resource) > + return this.name; This really shouldn't happen. If the jid provided is that broken, you should detect and handle it in the conversation constructor. @@ +403,5 @@ > + return this.name; > + return jid.resource; > + } > + > + if (!jid.node) Similarly here (I'm assuming the spec says a jid without a node is not allowed?) @@ +463,5 @@ > _targetResource: "", > get to() { > + if (!this._targetResource || this._isMucParticipant) > + return this.userName; > + return this._account.normalize(this.userName) + "/" + this._targetResource; Why do you need to add the normalize() call here? @@ +524,5 @@ > /* Called by the account when a messsage is received from the buddy */ > incomingMessage: function(aMsg, aStanza, aDate) { > let from = aStanza.attributes["from"]; > + if (!this._isMucParticipant) > + this._targetResource = this._account._parseJID(from).resource; Why did you add this if clause? Isn't it harmless if the targetResource is set to the nick? Just wondering. @@ +1393,5 @@ > + let convName = norm; > + > + // Checks if incoming message is a private muc. In this case we do not > + // need to normalize the jid as it will be in form room@domain/nick. > + if (this.isPrivateMUC(from)) I think if (this._mucs.has(norm)) would actually be simpler here. @@ +1433,5 @@ > conv.supportChatStateNotifications = !!state; > }, > > + // Checks if the received message is a private MUC message. > + isPrivateMUC: function(aFrom) { Will probably turn out not to be needed, as the callers generally already have normalize(aFrom) available in a local variable. @@ +1501,5 @@ > result.jid = jid; > return result; > }, > > + _parseMucJID: function(aJid) { It doesn't look like this is used after all, so you don't need to add it. Sorry I suggested it. @@ +1645,5 @@ > _MUCConversationConstructor: XMPPMUCConversation, > _accountBuddyConstructor: XMPPAccountBuddy, > > + // Create a new conversation. > + // The caller needs to check/perform normalizing before calling it if needed. Can't we be more tolerant about this and automatically normalize appropriately if needed? @@ +1650,5 @@ > + createConversation: function(aName) { > + if (!this._conv.has(aName)) { > + this._conv.set(aName, new this._conversationConstructor(this, aName)); > + if (this.isPrivateMUC(aName)) > + this._conv.get(aName)._isMucParticipant = true; It would be better to set this flag in the conversationConstructor. You can always add a parameter if you need to pass it.

(In reply to aleth [:aleth] from comment #9) > This really shouldn't happen. If the jid provided is that broken, you should > detect and handle it in the conversation constructor. > Similarly here (I'm assuming the spec says a jid without a node is not > allowed?) I know that this should not happen according to the spec, but I added them to keep result of shortName is correct in any case. > Why do you need to add the normalize() call here? To be sure that the case of appending will not happen, but this is also additional as if condition before it handles this case so I ignored this normalizing in this patch. > Why did you add this if clause? Isn't it harmless if the targetResource is > set to the nick? Just wondering. Because the value of targetResource will not be corresponding to its name. I removed this if clause and added a comment for targetResource in this patch. > Can't we be more tolerant about this and automatically normalize > appropriately if needed? This is done in this patch.

Comment on attachment 8625235 [details] [diff] [review] rev 3 - support private muc message Review of attachment 8625235 [details] [diff] [review]: ----------------------------------------------------------------- Looks much better overall. > (In reply to aleth [:aleth] from comment #9) > > This really shouldn't happen. If the jid provided is that broken, you should > > detect and handle it in the conversation constructor. > > Similarly here (I'm assuming the spec says a jid without a node is not > > allowed?) > > I know that this should not happen according to the spec, but I added them > to keep result of shortName is correct in any case. I liked the checks, but my suggestion was to move them to createConversation so that conversations with bad jids are never created in the first place. ::: chat/protocols/xmpp/xmpp.jsm @@ +371,5 @@ > _typingTimer: null, > supportChatStateNotifications: true, > _typingState: "active", > > + // Indicates that current conversation is a private muc messaging and the ...is with a MUC participant and the @@ +372,5 @@ > supportChatStateNotifications: true, > _typingState: "active", > > + // Indicates that current conversation is a private muc messaging and the > + // the recipient jid will be in form of room@domain/nick. recipient jid (stored in the userName) is of the form... @@ +383,5 @@ > > + // Returns jid (room@domain/nick) if it is a private muc, and the name of > + // conversation otherwise. > + get normalizedName() > + this._isMucParticipant ? this.userName.trim().toLowerCase() : this._account.normalize(this.name), this.name, not this.userName. If the trim().toLowerCase() combination (normalization for MUC participants) shows up more than once, a helper function would be a good idea. @@ +453,5 @@ > delete this._typingTimer; > } > }, > > + // holds the resource of user that you are currenty talking to, but in Nit: Generally start comments with a capital letter. @@ +466,5 @@ > > /* Called when the user enters a chat message */ > sendMsg: function(aMsg) { > + let notInRoom = (aError) => { > + let from = aError.stanza.attributes["from"]; The indentation is wrong here. @@ +554,5 @@ > this._account.removeConversation(this.normalizedName); > GenericConvIMPrototype.unInit.call(this); > } > }; > +function XMPPConversation(aAccount, aName, aMucParticipant) Let's add a comment saying we expect aName to be normalized here @@ +893,5 @@ > _init: function(aProtoInstance, aImAccount) { > GenericAccountPrototype._init.call(this, aProtoInstance, aImAccount); > > /* Ongoing conversations */ > + this._conv = new Map(); Add a comment saying that the keys of this._conv are assumed to be normalized like ... for normal conversations and like ... for MUC participant convs. @@ +1418,5 @@ > typingState = Ci.prplIConvIM.TYPING; > else if (state == "paused") > typingState = Ci.prplIConvIM.TYPED; > } > + let conv = this._conv.get(from); Is this always correct (for normal conversations too)? More generally, does the spec ensure that "from"s we pick up from incoming stanzas are normalized (i.e. lowercase)? Otherwise you may want to implement something like this._conv.safeGet... @@ +1626,5 @@ > + > + // Checks if conversation is a private muc. In this case we do not > + // need to normalize the name as it will be in form room@domain/nick. > + let isMucParticipant = this._mucs.has(convName); > + convName = isMucParticipant ? aName : convName; Need to normalize aName (trim and lowercase)

(In reply to aleth [:aleth] from comment #11) > Is this always correct (for normal conversations too)? No, I corrected that. > More generally, does the spec ensure that "from"s we pick up from incoming > stanzas are normalized (i.e. lowercase)? No, it does not.

Comment on attachment 8625357 [details] [diff] [review] rev 4 - support private muc message Review of attachment 8625357 [details] [diff] [review]: ----------------------------------------------------------------- Looks like this is close to being finished :-) ::: chat/protocols/xmpp/xmpp.jsm @@ +476,5 @@ > + let message; > + > + // XEP-0045 (7.5): MUC private messages. > + if (this._isMucParticipant && muc && !muc.left && > + aError.condition == "item-not-found") { Why the muc && !muc.left check here? Does that have anything to do with whether the participant is in the room or not? @@ +478,5 @@ > + // XEP-0045 (7.5): MUC private messages. > + if (this._isMucParticipant && muc && !muc.left && > + aError.condition == "item-not-found") { > + // If the recipient is not in the room. > + let nick = this._account._parseJID(from).resource; Why are we using 'from' from the stanza here and parsing it? We already know who we sent the message to, as this is a callback, so this can be simplified a lot. @@ +486,5 @@ > + else if (this._isMucParticipant) { > + // If the current account is not in the room. > + message = _("conversation.error.sendFailedAsNotInRoom", > + this._account.normalize(from), aMsg); > + } There is quite a bit of overhead here. Check if you can simplify this to what is essentially a map error.condition -> error message string label. @@ +487,5 @@ > + // If the current account is not in the room. > + message = _("conversation.error.sendFailedAsNotInRoom", > + this._account.normalize(from), aMsg); > + } > + else { This can be streamlined, you can move if (!this._isMucParticipant) return false; to the top. @@ +496,5 @@ > + this.writeMessage(from, message, {system: true, error: true}); > + return true; > + }; > + let errorHandler = this._account.handleErrors({itemNotFound: notInRoom, > + notAcceptable: notInRoom}); errorHandler is never used. Please unify the error handling here with the existing error handling for send message failures in incomingMessage, either by moving the additional error handling you are adding here to incomingMessage or the other way around. @@ +560,5 @@ > }; > + > +// Creates XMPP conversation. > +// We expect aName to be normalized. > +function XMPPConversation(aAccount, aName, aMucParticipant) Let's rename aName -> aNormalizedName, then you don't need the "we expect..." comment. @@ +1476,3 @@ > }, > > normalize: function(aJID) { Add a comment // Standard normalization for XMPP removes the resource part of jids. @@ +1477,5 @@ > > normalize: function(aJID) { > return aJID.trim() > .split("/", 1)[0] // up to first slash > .toLowerCase(); Rewrite this so it calls basicNormalize to avoid duplication. @@ +1482,5 @@ > }, > > + // Used for private MUC as jid is of the form room@domain/nick so we do not > + // need to split it like normal conversations. > + normalizeMucParticipant: function (aJID) { Let's call this basicNormalize instead, and move this block above normalize:... Nit: no space after function @@ +1645,5 @@ > + createConversation: function(aName) { > + let convName = this.normalize(aName); > + > + // Checks if conversation is a private MUC. In this case we do not > + // need to normalize the name as it will be in form room@domain/nick. ...is with a participant of a MUC we are in. ... ...we do not want to strip the resource as... @@ +1647,5 @@ > + > + // Checks if conversation is a private MUC. In this case we do not > + // need to normalize the name as it will be in form room@domain/nick. > + let isMucParticipant = this._mucs.has(convName); > + convName = isMucParticipant ? aName : convName; The else here is unnecessary. Just do if (isMucParticipant) convName = normalizeMucParticipant(aName)

(In reply to aleth [:aleth] from comment #13) > Why the muc && !muc.left check here? Does that have anything to do with > whether the participant is in the room or not? actually, we have 3 conditions we need to take care about them and they have the same error "item-not-found" and "not-acceptable". 1. If we try to send to participant not in a room we are in ("item-not-found"). 2. If we left a room and try to send to a participant in it ("not-acceptable"). 3. If we left the room so the room is removed and try to send to a participant in it ("item-not-found").

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #14) > actually, we have 3 conditions We also add to them 2 conditions for "item-not-found" and "not-acceptable" in normal conversation. eg. If the jid does not exist ("item-not-found").

Comment on attachment 8625598 [details] [diff] [review] rev 5 - support private muc message Review of attachment 8625598 [details] [diff] [review]: ----------------------------------------------------------------- Now that the normalization is figured out, getNormalizedChatBuddyName should be changed to use basicNormalize(...) just to be sure we are always consistent. ::: chat/protocols/xmpp/xmpp.jsm @@ +448,1 @@ > /* to, msg, state, attrib, data */ Let's remove this strange comment. @@ +448,3 @@ > /* to, msg, state, attrib, data */ > let s = Stanza.message(this.to, null, aNewState); > + this._account.sendStanza(s, errorHandler); Because typing notifications aren't that important, we could simplify this to // We don't care about errors in response to typing notifications // (e.g. because the user has left the room when talking to a MUC // participant). this._account.sendStanza(s, () => true); @@ +495,5 @@ > /* Called by the account when a messsage is received from the buddy */ > incomingMessage: function(aMsg, aStanza, aDate) { > let from = aStanza.attributes["from"]; > + let norm = this._account.normalize(from); > + let muc = this._account._mucs.get(norm); Move these inside if(error) as they are not needed otherwise. @@ +506,5 @@ > if (error.condition == "remote-server-not-found") > aMsg = _("conversation.error.remoteServerNotFound"); > else > aMsg = _("conversation.error.unknownError"); > + aMsg = _("conversation.error.notDelivered", aMsg); This line looks like it shouldn't be here. @@ +511,2 @@ > } > + else { You don't need the else {} block wrapping this @@ +1639,5 @@ > + // Checks if conversation is with a participant of a MUC we are in. We do > + // not want to strip the resource as it is of the form room@domain/nick. > + let isMucParticipant = this._mucs.has(convName); > + if (isMucParticipant) > + convName = aName; = basicNormalize(aName)

Comment on attachment 8626012 [details] [diff] [review] rev 6 - support private muc message Review of attachment 8626012 [details] [diff] [review]: ----------------------------------------------------------------- Works well so far! Joining jdev@conference.jabber.org, I was a little surprised there are participants with * nicks with uppercase letters * nicks with spaces in them Before we land this, we should double check we are doing the right thing here by lowercasing everything, and that nothing breaks when using nicks with spaces. Please find out from the spec if jids are case sensitive or not, and where spaces are allowed. Are the rules for resources/full Jids different than for base jids? ::: chat/protocols/xmpp/xmpp.jsm @@ +489,5 @@ > aMsg = _("conversation.error.remoteServerNotFound"); > else > aMsg = _("conversation.error.unknownError"); > } > + if (this._isMucParticipant && muc && !muc.left && else if

(In reply to aleth [:aleth] from comment #19) > Please find out from the spec if jids are case sensitive or not, and where > spaces are allowed. Are the rules for resources/full Jids different than for > base jids? For example, the kind of thing I am wondering about is if the nicks "UPPER" and "upper" (try to) join the same room. Are they treated as the same participant, or are they two different participants?

11:31:54 - aleth: Which parts of a full jid are case sensitive? And which parts can contain spaces? (only the resource?) 12:02:35 - Link Mauve: aleth, only the resource, yeah. 12:02:39 - Link Mauve: (For both.) 12:05:12 - aleth: Link Mauve: OK, thanks! 12:09:19 - Link Mauve: aleth, actually that’s not totally true, as per [xep 106] you can use \20 in the node part to encode a space. 12:10:11 - Link Mauve: For example I could use link\20mauve@example.com/some_resource as my JID, and clients should (but I don’t know how widespread this XEP is implemented) display it as “link mauve@example.com/some_resource”. 12:10:19 - Link Mauve: http://xmpp.org/extensions/xep-0106.html 12:10:38 - aleth: Thanks. Using a node like that does seem to be asking for trouble... 12:11:13 - aleth: Link Mauve: XEP-106 is only a draft though? 12:11:18 - dwd: If it's always presented with the \20 it's not a problem. 12:11:44 - dwd: aleth, Oh, no, "Draft" is better than "Experimental". It's only one step before "Final". 12:12:13 - Link Mauve: See http://xmpp.org/extensions/xep-0001.html#approval-std 12:12:18 - aleth: OK 12:12:41 - dwd: I suspect moving that one to Final wouldn't be a problem, you'd just need to find two implementations. 12:13:13 - dwd: Although it's untouched since 2007, so might need a tidy-up (like referring to 6120 instead of 3920). 12:13:35 - Zash: Spectrum does \escapes iirc 12:13:54 - Link Mauve: SleekXMPP does unescaping. 12:13:59 - Zash: Prosody probably has code for it somewhere too 12:14:00 - Link Mauve: (And thus slixmpp.) It would be good to confirm this answer with the specs and then change normalize/basicNormalize accordingly. You don't have to implement XEP-0106 for this bug.

(In reply to aleth [:aleth] from comment #19) > Please find out from the spec if jids are case sensitive or not, and where > spaces are allowed. Are the rules for resources/full Jids different than for > base jids? According to spec XEP-0029 [1], we were breaking some of it. Although this spec is retracted, it's used. I found out that after testing with some services. jid = node@domain/resource For node: case is preserved, but comparisons will be made in case-normalized. For domain: case-insensitive. For resource: case-sensitive. may be the role of basicNormalize will be only in trimming. [1] http://xmpp.org/extensions/xep-0029.html#sect-idp666800

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #22) > (In reply to aleth [:aleth] from comment #19) > > Please find out from the spec if jids are case sensitive or not, and where > > spaces are allowed. Are the rules for resources/full Jids different than for > > base jids? > > According to spec XEP-0029 [1], we were breaking some of it. > Although this spec is retracted, it's used. I found out that after testing > with some services. That's right. The current spec is rfc 6122 should there be any issues, but it shouldn't contradict 0029 much. > jid = node@domain/resource > For node: case is preserved, but comparisons will be made in case-normalized. > For domain: case-insensitive. > For resource: case-sensitive. > > may be the role of basicNormalize will be only in trimming. How about renaming basicNormalize to normalizeFullJid? It should trim and then lowercase the node and domain parts. Probably doesn't make sense for normalize to call it any more though.

Comment on attachment 8626411 [details] [diff] [review] rev 7 - support private muc message Review of attachment 8626411 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1507,3 @@ > normalize: function(aJID) { > + // Up to first slash. > + return aJID ? aJID.split("/", 1)[0].toLowerCase() : undefined; Is there any case in the code where you think aJid can be undefined, or are you just being careful?

(In reply to aleth [:aleth] from comment #25) > Is there any case in the code where you think aJid can be undefined, or are > you just being careful? Just being careful.

Comment on attachment 8626411 [details] [diff] [review] rev 7 - support private muc message Review of attachment 8626411 [details] [diff] [review]: ----------------------------------------------------------------- Almost done! :-) ::: chat/protocols/xmpp/xmpp.jsm @@ +1495,5 @@ > buddy.vCardFormattedName = ""; > buddy._vCardReceived = true; > }, > > + normalizeFullJid: function(aJID) { Add a comment above this, something like // The node and domain are lowercase, while resources are case sensitive and can contain spaces with a reference to the specs. @@ +1500,5 @@ > + if (!aJID) > + return undefined; > + let jid = this._parseJID(aJID.trim()); > + return jid ? jid.jid : undefined; > + }, Let's shorten this to normalizeFullJid: function(aJid) this._parseJID(aJID.trim()).jid, The reason is that if it's called with an undefined aJID, then we have a bug somewhere. So if we handle that case here and return undefined, we're just shifting the error that will happen to the calling function. Nothing is gained. @@ +1507,3 @@ > normalize: function(aJID) { > + // Up to first slash. > + return aJID ? aJID.split("/", 1)[0].toLowerCase() : undefined; Please put back the trim, and similarly we can do without the undefined check.

Comment on attachment 8626722 [details] [diff] [review] rev 8 - support private muc message Review of attachment 8626722 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1496,5 @@ > buddy._vCardReceived = true; > }, > > + // The node and domain are lowercase, while resources are case sensitive and > + // can contain spaces with a reference to the specs. I meant you should mention the RFC/XEP numbers in the comment ;)

> I meant you should mention the RFC/XEP numbers in the comment ;) Ah, sorry. I haven't noticed that.

Comment on attachment 8626746 [details] [diff] [review] rev 9 - support private muc message Review of attachment 8626746 [details] [diff] [review]: ----------------------------------------------------------------- Let's try this in nightlies!