IB fails silently trying to join such rooms. Pidgin seems to show errors in this case like "407: Registration Required". Possibly part of capability discovery.

Comment on attachment 8574111 [details] [diff] [review] rev 1 - discover error in conversation and show it Review of attachment 8574111 [details] [diff] [review]: ----------------------------------------------------------------- The commit message should be more specific (you don't discover all conversation errors, after all). Maybe "Handle errors on attempting to join private XMPP rooms"? ::: chat/locales/en-US/xmpp.properties @@ +46,5 @@ > conversation.error.joinFailed=Could not join: %S > # These are displayed in a conversation as a system error message. > conversation.error.remoteServerNotFound=Could not reach the recipient's server > conversation.error.unknownError=Unknown error > +conversation.error.notAuthorized=Registration Required How about "Registration required: You are not authorized to join this room." Please move the string under the joinFailed string. ::: chat/protocols/xmpp/xmpp.jsm @@ +992,5 @@ > > // The join failed. > if (muc.left && aStanza.attributes["type"] == "error") { > + let error = this.parseError(aStanza); > + let message = ""; You don't need the = "" as you have a default in the switch statement. @@ +993,5 @@ > // The join failed. > if (muc.left && aStanza.attributes["type"] == "error") { > + let error = this.parseError(aStanza); > + let message = ""; > + if (error) { You don't need to check this as you know that type == "error" before the parseError call, so it's always true. @@ +996,5 @@ > + let message = ""; > + if (error) { > + switch (error.condition) { > + case "not-authorized": > + message = _("conversation.error.notAuthorized"); Let's call this string "joinFailedNotAuthorized" so we can make the string shown to the user more specific. @@ +1005,5 @@ > + } > + } > + else { > + message = _("conversation.error.joinFailed", muc.name); > + this.ERROR("Failed to join MUC: " + aStanza.convertToString()); Move this to the default: case and change it to this.ERROR(`Failed to join MUC due to unhandled ${error.condition} error.`); The point of this is to make it clear in the debug log what needs to be implemented to better take care of this if it happens.

Comment on attachment 8574141 [details] [diff] [review] rev 2 - Handle errors on attempting to join private XMPP rooms Review of attachment 8574141 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1007,1 @@ > {system: true, error: true}); Does this fit on one line now? (<= 80 characters)

(In reply to aleth [:aleth] from comment #4) > Comment on attachment 8574141 [details] [diff] [review] > rev 2 - Handle errors on attempting to join private XMPP rooms > > Review of attachment 8574141 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: chat/protocols/xmpp/xmpp.jsm > @@ +1007,1 @@ > > {system: true, error: true}); > > Does this fit on one line now? (<= 80 characters) Yes, it fits.

Comment on attachment 8574150 [details] [diff] [review] rev 3 - Handle errors on attempting to join private XMPP rooms Review of attachment 8574150 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good now!

https://hg.mozilla.org/comm-central/rev/8b047ada221f Thanks!