This won't be an issue until we switch over to JS-XMPP, but there are (minor) issues when connecting already existing XMPP accounts after removing the override pref. For example, the resource in libpurple is part of the account name, and doesn't get set correctly when connected with JS-XMPP. It might be worth checking other advanced prefs (e.g. encryption) migrate correctly.

(In reply to aleth from comment #0) > the resource in libpurple is part of the > account name, and doesn't get set correctly when connected with JS-XMPP. This is handled at http://mxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#693 695 this._jid = this._parseJID(this.name); 696 697 // For the resource, if the user has edited the option to a non 698 // empty value, use that. 699 if (this.prefs.prefHasUserValue("resource")) { 700 let resource = this.getString("resource"); 701 if (resource) 702 this._jid.resource = resource; 703 } 704 // Otherwise, if the username doesn't contain a resource, use the 705 // value of the resource option (it will be the default value). 706 // If we set an empty resource, XMPPSession will fallback to 707 // XMPPDefaultResource (set to brandShortName). 708 if (!this._jid.resource) 709 this._jid.resource = this.getString("resource");

Looks like this was assigned to me because I still had a patch for it in my queue.

Reformatted to make better use of 80 chars...

Comment on attachment 8757406 [details] [diff] [review] Set resource pref for migrated libpurple XMPP accounts Review of attachment 8757406 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1193,3 @@ > this._jid.resource = this.getString("resource"); > + else if (this._jid.resource) > + this.prefs.setCharPref("resource", this._jid.resource); Can this contain non-ascii characters?

Comment on attachment 8757406 [details] [diff] [review] Set resource pref for migrated libpurple XMPP accounts Review of attachment 8757406 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp.jsm @@ +1193,3 @@ > this._jid.resource = this.getString("resource"); > + else if (this._jid.resource) > + this.prefs.setCharPref("resource", this._jid.resource); getString uses getComplexValue, so we definitely want to set it using the same type of mechanism: https://dxr.mozilla.org/comm-central/source/chat/modules/jsProtoHelper.jsm#220

Good catch! Unicode is probably allowed, but unlikely. (I also tweaked the comment, hopefully an improvement)

Fixed incorrect use of nsISupportsString

Comment on attachment 8764621 [details] [diff] [review] Set resource pref for migrated libpurple XMPP accounts Review of attachment 8764621 [details] [diff] [review]: ----------------------------------------------------------------- r+ assuming my understanding is fixed and the nits are fixed. ::: chat/protocols/xmpp/xmpp.jsm @@ +1183,5 @@ > this._jid = this._parseJID(this.name); > > + // For the resource, if the user has edited the option, always use that. > + // If that would set an empty resource, XMPPSession will fallback to > + // XMPPDefaultResource (set to brandShortName). To make sure I understand, at the end of this block of code, it's OK if this._jid.resource is False-y because it gets set to brandShortname later on? @@ +1188,5 @@ > + // If no resource is set by the user, the pref getter will return a > + // default value. > + if (this.prefs.prefHasUserValue("resource") || !this._jid.resource) > + this._jid.resource = this.getString("resource"); > + else if (this._jid.resource) { Isn't this just an else? @@ +1193,5 @@ > + // If there is a resource in the account name (inherited from libpurple), > + // migrate it to the pref so it appears correctly in the advanced account > + // options next time. > + let str = Cc["@mozilla.org/supports-string;1"] > + .createInstance(Ci.nsISupportsString); Nit: Line up the. with the [.

aleth, I think you need to upload a new patch.

Nit fixed.

(In reply to Patrick Cloke [:clokep] from comment #8) > > + // For the resource, if the user has edited the option, always use that. > > + // If that would set an empty resource, XMPPSession will fallback to > > + // XMPPDefaultResource (set to brandShortName). > > To make sure I understand, at the end of this block of code, it's OK if > this._jid.resource is False-y because it gets set to brandShortname later on? Yes. > @@ +1188,5 @@ > > + // If no resource is set by the user, the pref getter will return a > > + // default value. > > + if (this.prefs.prefHasUserValue("resource") || !this._jid.resource) > > + this._jid.resource = this.getString("resource"); > > + else if (this._jid.resource) { > > Isn't this just an else? I preferred to keep it explicit as it's confusing enough as it is.

(In reply to Patrick Cloke [:clokep] from comment #9) > aleth, I think you need to upload a new patch. bzexport fail ;)

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