Remove _chatRoomFieldsList from IRC code

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: clokep, Assigned: clokep)

Tracking

trunk
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

5 years ago
This has become obsolete, we can now directly set the chatRoomFields now that joinChat returns a conversation object for IRC (see bug 1003200).
Assignee

Comment 1

5 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #8424812 - Flags: review?(aleth)
Assignee

Comment 2

5 years ago
This blocks bug 955366 since this code should be removed or switched to a NormalizedMap.
Blocks: 955366

Comment 3

5 years ago
Comment on attachment 8424812 [details] [diff] [review]
Patch v1

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

Well spotted! This is a nice simplification and improves the error handling too.

::: chat/protocols/irc/ircBase.jsm
@@ +1086,1 @@
>        return conversationErrorMessage(this, aMessage, "error.noChannel");

It's not strictly speaking part of this bug, but as you are touching all these connection errors anyway... please add a conv.joining = false to the errors which imply joining failed.

@@ +1096,1 @@
>        return serverErrorMessage(this, aMessage,

Shouldn't this be a conversationErrorMessage now we have a tab open despite not being able to join the channel?

You also have to remove chatRoomFields on the conversation so it doesn't reconnect.

@@ +1201,5 @@
>      },
>      "437": function(aMessage) { // ERR_UNAVAILRESOURCE
>        // <nick/channel> :Nick/channel is temporarily unavailable
> +      conversationErrorMessage(this, aMessage, "error.unavailable");
> +      return true;

Are you intentionally allowing reconnections here due to the "temporarily"?

@@ +1285,5 @@
>        return false;
>      },
>      "471": function(aMessage) { // ERR_CHANNELISFULL
>        // <channel> :Cannot join channel (+l)
> +      conversationErrorMessage(this, aMessage, "error.channelFull");

ditto

@@ +1295,5 @@
>        return false;
>      },
>      "473": function(aMessage) { // ERR_INVITEONLYCHAN
>        // <channel> :Cannot join channel (+i)
> +      conversationErrorMessage(this, aMessage, "error.inviteOnly");

I don't think we want reconnections here.
Attachment #8424812 - Flags: review?(aleth) → review-
Assignee

Comment 4

5 years ago
(In reply to aleth [:aleth] from comment #3)
> ::: chat/protocols/irc/ircBase.jsm
> @@ +1086,1 @@
> >        return conversationErrorMessage(this, aMessage, "error.noChannel");
> 
> It's not strictly speaking part of this bug, but as you are touching all
> these connection errors anyway... please add a conv.joining = false to the
> errors which imply joining failed.
Good point! I should probably abstract this out better.

> @@ +1096,1 @@
> >        return serverErrorMessage(this, aMessage,
> 
> Shouldn't this be a conversationErrorMessage now we have a tab open despite
> not being able to join the channel?
Probably, serverErrorMessage shouldn't really be used anyway.

> You also have to remove chatRoomFields on the conversation so it doesn't
> reconnect.
No. I want this to attempt to reconnect. The user might have closed other channels, etc.

> @@ +1201,5 @@
> >      "437": function(aMessage) { // ERR_UNAVAILRESOURCE
> >        // <nick/channel> :Nick/channel is temporarily unavailable
> > +      conversationErrorMessage(this, aMessage, "error.unavailable");
> > +      return true;
> 
> Are you intentionally allowing reconnections here due to the "temporarily"?
Yes. This can also be called with a nick, by the way.

> 
> @@ +1285,5 @@
> >      "471": function(aMessage) { // ERR_CHANNELISFULL
> >        // <channel> :Cannot join channel (+l)
> > +      conversationErrorMessage(this, aMessage, "error.channelFull");
> 
> ditto
Yes.

> @@ +1295,5 @@
> >        return false;
> >      },
> >      "473": function(aMessage) { // ERR_INVITEONLYCHAN
> >        // <channel> :Cannot join channel (+i)
> > +      conversationErrorMessage(this, aMessage, "error.inviteOnly");
> 
> I don't think we want reconnections here.
You're probably right, I was very iffy on this one. I'll delete _chatRoomFields here.
Assignee

Comment 5

5 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #8424812 - Attachment is obsolete: true
Attachment #8425426 - Flags: review?(aleth)

Comment 6

5 years ago
Comment on attachment 8425426 [details] [diff] [review]
Patch v2

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

::: chat/protocols/irc/ircUtils.jsm
@@ +218,5 @@
>  
> +// Print an error message into a conversation, optionally mark the conversation
> +// as not joined and/or not rejoinable.
> +function conversationErrorMessage(aAccount, aMessage, aError,
> +                                  aJoining, aRejoinable = true) {

I think this would be clearer with aJoinFailed = false as the default (when it's true, joining is set to false). I can't see any use case for aJoining = true and passing undefined as a parameter always looks weird.
Attachment #8425426 - Flags: review?(aleth) → review-
Assignee

Comment 7

5 years ago
Posted patch Patch v3 (obsolete) — Splinter Review
The double negatives make my head hurt slightly, but I agree that passing undefined always seems "wrong".
Attachment #8425426 - Attachment is obsolete: true
Attachment #8425454 - Flags: review?(aleth)

Comment 8

5 years ago
Comment on attachment 8425454 [details] [diff] [review]
Patch v3

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

Yeah, double negatives are unfortunate but better than the alternative.

Looks good, thanks!
Attachment #8425454 - Flags: review?(aleth) → review+

Comment 9

5 years ago
Comment on attachment 8425454 [details] [diff] [review]
Patch v3

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

::: chat/protocols/irc/ircUtils.jsm
@@ +225,5 @@
>    conv.writeMessage(aMessage.servername, _(aError, aMessage.params[1]),
>                      {error: true, system: true});
>    delete conv._pendingMessage;
> +
> +  // If a value for joining is explictly given, mark it.

Nit: you could save yourself the isMuc if you did

if (aAccount.isMUCName){ 
  if (aJoinfailed)
    ...
  if (!rejoinable)
    ...
}

Not going to r- for this though.
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 10

5 years ago
Posted patch Patch v4Splinter Review
Carrying the review forward for changing the nit.
Attachment #8425454 - Attachment is obsolete: true
Attachment #8425529 - Flags: review+
Assignee

Comment 11

5 years ago
https://hg.mozilla.org/comm-central/rev/0568cdefd554
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.