1012660 - Remove _chatRoomFieldsList from IRC code

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

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

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch v1

Comment 2

[Patrick Cloke [:clokep]]

This blocks bug 955366 since this code should be removed or switched to a NormalizedMap.

Comment 3

[aleth [:aleth]]

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.

Comment 4

[Patrick Cloke [:clokep]]

(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.

Comment 5

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Comment 6

[aleth [:aleth]]

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.

Comment 7

[Patrick Cloke [:clokep]]

Attachment: Patch v3

The double negatives make my head hurt slightly, but I agree that passing undefined always seems "wrong".

Comment 8

[aleth [:aleth]]

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!

Comment 9

[aleth [:aleth]]

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.

Comment 10

[Patrick Cloke [:clokep]]

Attachment: Patch v4

Carrying the review forward for changing the nit.

Comment 11

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/0568cdefd554