Use Maps and Sets in XMPP code

RESOLVED FIXED in 1.6

Status

Chat Core
XMPP
--
minor
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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).
(Assignee)

Comment 1

4 years ago
Created attachment 8432237 [details] [diff] [review]
Patch

aleth, if you feel you don't know the XMPP code well enough, please feel free to forward it to Florian. Thanks!
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8432237 - Flags: review?(aleth)
(Assignee)

Comment 2

4 years ago
This depends on bug 1017946 because of changes, not really because of functionality.
Depends on: 1017946

Updated

4 years ago
Depends on: 1014472
Blocks: 1011226

Comment 3

4 years ago
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.
Attachment #8432237 - Flags: review?(aleth) → review-
(Assignee)

Comment 4

4 years ago
Created attachment 8453336 [details] [diff] [review]
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.
Attachment #8432237 - Attachment is obsolete: true
Attachment #8453336 - Flags: review?(aleth)

Comment 5

4 years ago
Comment on attachment 8453336 [details] [diff] [review]
Patch v2

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

Thanks!
Attachment #8453336 - Flags: review?(aleth) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/comm-central/rev/fd75bab04b96
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
(Assignee)

Comment 7

4 years ago
Created attachment 8455614 [details] [diff] [review]
Minor fix aleth found by inspection
Attachment #8455614 - Flags: review?(aleth)

Updated

4 years ago
Attachment #8455614 - Flags: review?(aleth) → review+
(Assignee)

Updated

4 years ago
Depends on: 1044172

Updated

3 years ago
Depends on: 1175462
You need to log in before you can comment on or make changes to this bug.