Closed
Bug 1018771
Opened 10 years ago
Closed 10 years ago
Use Maps and Sets in XMPP code
Categories
(Chat Core :: XMPP, defect)
Chat Core
XMPP
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: clokep, Assigned: clokep)
References
Details
Attachments
(2 files, 1 obsolete file)
18.23 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
1.28 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
aleth, if you feel you don't know the XMPP code well enough, please feel free to forward it to Florian. Thanks!
Assignee | ||
Comment 2•10 years ago
|
||
This depends on bug 1017946 because of changes, not really because of functionality.
Depends on: 1017946
Comment 3•10 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•10 years ago
|
||
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•10 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•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/fd75bab04b96
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8455614 -
Flags: review?(aleth)
Updated•10 years ago
|
Attachment #8455614 -
Flags: review?(aleth) → review+
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/1c931c239cbb
You need to log in
before you can comment on or make changes to this bug.
Description
•