Improve XMPP handleErrors function to display error system messages more easily

RESOLVED FIXED in Instantbird 44

Status

Chat Core
XMPP
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

trunk
Instantbird 44

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Now there's quite a bit of error handling in XMPP, it's clear the most common case of an error handling function is one that simply displays a localised error system message. We should be able to remove a lot of code duplication by adding this case to  handleErrors (the error-handler-callback-generating helper function).
(Assignee)

Comment 1

3 years ago
Created attachment 8663137 [details] [diff] [review]
errorhandling.diff

See amended comment above handleErrors() in the patch for details.

Suggestions for improvements/amendments welcome!
Attachment #8663137 - Flags: review?(clokep)
(Assignee)

Comment 2

3 years ago
Created attachment 8663140 [details] [diff] [review]
errorhandling.diff v2

A bit more readable, I think.
Attachment #8663137 - Attachment is obsolete: true
Attachment #8663137 - Flags: review?(clokep)
Attachment #8663140 - Flags: review?(clokep)
Comment on attachment 8663140 [details] [diff] [review]
errorhandling.diff v2

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

Minor nits. Thanks for cleaning this up!

::: chat/protocols/xmpp/xmpp.jsm
@@ +481,2 @@
>        // TODO: We should then discover user's reserved nickname (it could be
> +      // discovered before joining a room): XEP-0045 (7.12): Discovering

Was this comment purposefully wrapped this way? This sentence seems to have three colons in it. I find that confusing. :)

@@ +1396,1 @@
>    handleErrors(aHandlers, aThis) {

Can we add some (vertical) white space into this function? It looks like there's three separate sections (each separated by a return false). (I'd also like a brief comment that reiterates the block comment about what each section is doing. :))
Attachment #8663140 - Flags: review?(clokep) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8664559 [details] [diff] [review]
errorhandling.diff v3

Nits fixed and inlined some helper vars.
Assignee: nobody → aleth
Attachment #8663140 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8664559 - Flags: review?(clokep)
Comment on attachment 8664559 [details] [diff] [review]
errorhandling.diff v3

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

Thanks!
Attachment #8664559 - Flags: review?(clokep) → review+
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/comm-central/rev/a5679881ec4c40a1e2441ee27cb5d07db70c9a5e
Bug 1206238 - Improve XMPP handleErrors to display error system messages directly. r=clokep
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 44
You need to log in before you can comment on or make changes to this bug.