1018771 - Use Maps and Sets in XMPP code

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Patrick Cloke [:clokep]]

This is kind of a clone in bug 955366, kind of a port of bug 1003200, kind of some other clean up. I'm hesitant to say this is entirely necessary, but it seems safer / better to use Maps than objects for data storage.

This also adds the joining throbber for XMPP, which requires quite a bit of reorganization (actually creating and returning the conversation from joinChat, first of all).

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch

aleth, if you feel you don't know the XMPP code well enough, please feel free to forward it to Florian. Thanks!

Comment 2

[Patrick Cloke [:clokep]]

This depends on bug 1017946 because of changes, not really because of functionality.

Comment 3

[aleth [:aleth]]

Comment on attachment 8432237 [details] [diff] [review]
Patch

Review of attachment 8432237 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup!

::: chat/protocols/xmpp/xmpp.jsm
@@ +665,5 @@
> +    let muc = new this._MUCConversationConstructor(this, jid, nick);
> +    this._mucs.set(jid, muc);
> +    // Store the prplIChatRoomFieldValues to enable later reconnections.
> +    muc._chatRoomFields = aComponents;
> +    muc.joining = true;

I think you should also set muc.left = true here, or better, override the left = false default value from jsProtoHelper.

@@ +854,5 @@
> +      let muc = this._mucs.get(jid);
> +      muc.joining = false;
> +
> +      // The join failed.
> +      if (muc.left && aStanza.attributes["type"] == "error") {

Not sure what the muc.left test is trying to do here? Needs a better comment at least.

@@ +855,5 @@
> +      muc.joining = false;
> +
> +      // The join failed.
> +      if (muc.left && aStanza.attributes["type"] == "error") {
> +        this.ERROR("Failed to join MUC: " + aStanza.convertToString());

Now we have a user-visible tab open we should display an error message in it, otherwise it will just be blank.

Comment 4

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Updated patch that unbitrots and takes into account review comments.

This does NOT update the comment as discussed on IRC:
I assume this code block is only supposed to be entered if a join fails and not if any error stanza is received. I'm unsure if there are other situations in which we receive an error stanza for a MUC.

Comment 5

[aleth [:aleth]]

Comment on attachment 8453336 [details] [diff] [review]
Patch v2

Review of attachment 8453336 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 6

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/fd75bab04b96

Comment 7

[Patrick Cloke [:clokep]]

Attachment: Minor fix aleth found by inspection

Comment 8

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/1c931c239cbb