953944 - Implement IRC in JavaScript

Status: RESOLVED FIXED

(Chat Core :: IRC, enhancement)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2010-09-15 19:01:00 UTC ***

Tracking bug to reimplement IRC in JavaScript in order to dogfood the js-proto capabilities.

Work is being done in the experiments repository in Mercurial.

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2010-09-15 19:44:08 UTC ***

I also intend to add some other IRC features to the UI after the backend is rewritten.

Comment 2

[Patrick Cloke [:clokep]]

Attachment: Beta 1 of JavaScript IRC

*** Original post on bio 507 as attmnt 526 at 2011-02-20 22:53:00 UTC ***

I'm attaching an extension which will overwrite the libpurple IRC with an JavaScript version.  Note that commands do not work yet on this and some other random features, but it should be mostly usable (you'll have to use the Join chat menu to open rooms though).

Also, this will spit out A LOT of garbage onto the error console, but if you crash please send me the error as well as the dumped messages before it -- they should give me all the information I need to reproduce.

The one command that people might actually miss is the /nick command...sorry about that...I'm working on it.

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2011-02-21 00:51:08 UTC ***

A quick note that this will automatically work with the NickServ on at least irc.mozilla.org (the PASS command seems to get forwarded to the NickServ). A new version should be up soon with commands working.

Comment 4

[Patrick Cloke [:clokep]]

Attachment: Beta 2 of JavaScript IRC

*** Original post on bio 507 as attmnt 528 at 2011-02-21 03:00:00 UTC ***

This version contains some commands (/nick, /join, /kick), although I can't promise they all work under all situations. The commands are listed at https://wiki.instantbird.org/IRC_Commands although they aren't all implemented yet.

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8352267 [details]
Beta 1 of JavaScript IRC

*** Original change on bio 507 attmnt 526 at 2011-02-21 03:00:18 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Beta 3 of JavaScript IRC

*** Original post on bio 507 as attmnt 543 at 2011-02-26 19:35:00 UTC ***

Since Mic found so many bugs in the Beta 2, I decided to release another one with a lot of bugs fixes and some basic support for CTCP, by "basic support" (/me, /action, /version, that's really it...) Seems to work pretty well though for me.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8352269 [details]
Beta 2 of JavaScript IRC

*** Original change on bio 507 attmnt 528 at 2011-02-26 19:35:54 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Patrick Cloke [:clokep]]

Attachment: Beta 3 R2 of JavaScript IRC (updated for trunk)

*** Original post on bio 507 as attmnt 576 at 2011-04-04 00:26:00 UTC ***

Some changes to jsProtoHelper and the IM services are reflected in this update.

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8352284 [details]
Beta 3 of JavaScript IRC

*** Original change on bio 507 attmnt 543 at 2011-04-04 00:26:41 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 10

[Florian Quèze [:florian]]

*** Original post on bio 507 at 2011-05-23 16:31:21 UTC ***

It's too late to start working on this for 0.3.

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8352318 [details]
Beta 3 R2 of JavaScript IRC (updated for trunk)

*** Original change on bio 507 attmnt 576 at 2011-08-30 16:46:18 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Patrick Cloke [:clokep]]

Attachment: Initial patch v0.1

*** Original post on bio 507 as attmnt 1006 at 2011-11-18 01:57:00 UTC ***

This is my initial transition of IRC from an extension to a patch. It's working now, but definitely needs some work still.

This patch does not currently include removing purple/libpurple/protocols/irc or any other changes in purple/, nor does it include my irc doc/ directory.

To Do:
 * Scope the modules IRC is using (either by a naming scheme (irc prefix) or putting them in a separate folder)
 * Rename the poorly named modules
 * Further verify that "stuff" works
 * Handle some CTCP formatting characters
 * Remove some unused files that are place holders (the dcc files, etc.)

Comment 13

[Patrick Cloke [:clokep]]

Attachment: WIP 2

*** Original post on bio 507 as attmnt 1041 at 2011-11-30 11:37:00 UTC ***

(In reply to comment #8)
> To Do:
>  * Scope the modules IRC is using (either by a naming scheme (irc prefix) or
> putting them in a separate folder)
>  * Rename the poorly named modules
>  * Further verify that "stuff" works
>  * Handle some CTCP formatting characters
>  * Remove some unused files that are place holders (the dcc files, etc.)

This updated patch puts an "irc" prefix before all the files and removes unused files.

It has some basic code for handling CTCP formatting, which doesn't work right now.

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8352748 [details] [diff] [review]
Initial patch v0.1

*** Original change on bio 507 attmnt 1006 at 2011-11-30 11:37:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Patrick Cloke [:clokep]]

Attachment: WIP 3

*** Original post on bio 507 as attmnt 1047 at 2011-12-03 03:16:00 UTC ***

This handles parts of a first review from flo over IRC.

Fixes
* } else styling
* Removing dead code
* Moving some functions out of the utils file that were only moved once
* Simplifying some logic
* Using the global hasOwnProperty function in some places (more to do!)
* Cleaned up the CTCP low/high level dequote functions (with help from Mook)

Other things added to the to do list:
* l10n!
* More clean up

I'm mostly attaching this patch as a save point before I start localization.

Comment 16

[Patrick Cloke [:clokep]]

Comment on attachment 8352783 [details] [diff] [review]
WIP 2

*** Original change on bio 507 attmnt 1041 at 2011-12-03 03:16:18 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[Instantbot]

*** Original post on bio 507 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-12-03 06:19:03 UTC ***

Drive-by review; remember that all I can do is pick nits, since I don't know
the code well.  Also, my eyes kinda glazed over around all the numeric codes. 
I'm just suggesting style changes; please feel free to ignore anything you
disagree with.

(from review of attachment 8352783 [details] [diff] [review] (bio-attmnt 1041))
>diff --git a/chat/protocols/irc/Makefile.in b/chat/protocols/irc/Makefile.in

>+++ b/chat/protocols/irc/Makefile.in

>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 1998
>+# the Initial Developer. All Rights Reserved.
That seems unlikely.

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js

>+function rfc2812Message(aData) {
>+  LOG(aData);
>+  let message = {rawMessage: aData};
>+  let temp;
>+
>+  // Splits the raw string into four parts (the second is required)
>+  //   source
>+  //   command
>+  //   [parameter]
>+  //   [:last paramter]
>+  // See http://joshualuckers.nl/2010/01/10/regular-expression-to-match-raw-irc-messages/
>+  // Should be equivalent to the slightly simplier:
>+  //   /^(?:[:@](\S+) )?(\S+)(?: ((?:[^: ]\S* ?)*))?(?: ?:(.*))?$/
>+  if ((temp = aData.match(/^(?:[:@]([^ ]+) )?([^ ]+)(?: ((?:[^: ][^ ]* ?)*))?(?: ?:(.*))?$/))) {
this is probably clearer as two statements:

  let temp = aData.match(/..../);
  if (temp) {
    ...
speaking as somebody who doesn't really speak IRC: even better if this isn't an
RE, or split into several, or a more explicit BNF in the comment.

Yeah, I'd probably rewrite this as
  1. Pull out source, if aData[0] is ':' or '@' (and do the nick splitting)
  2. pull out command = aData.split(' ', 1);
  3. pull out last param via aData.lastIndexOf(':')
  4. pull out the othere params via aData.split(' ');

>+Chat.prototype = {

>+  sendMsg: function(aMessage) {
>+    // Only send message if we're in the room
>+    // XXX is this the expected behavior?
I believe some IRC bouncers will confuse you here - they prefix your nick with
something-or-other (and I can't remember what), so it looks like you're not in
the room (because what you think your nick is isn't there).

Silver (from chatzilla) dislikes those bouncers because of all the
spec-breaking they do, of course :p
>+    if (this._hasParticipant(this._account._nickname)) {
>+      this._account._sendMessage("PRIVMSG", [aMessage], this.name);
>+      this.writeMessage(this._account._nickname, aMessage, {outgoing: true});
add a comment that this is needed because the server won't send the message
back to you. (and if you change the participant check before, move it here.)

>+  unInit: function() {
>+    if (this._account.connectionState == Ci.purpleIAccount.STATE_CONNECTED)
>+      this._account._sendMessage("PART", [this.name]);
I hope the server can deal with people PARTing channels they're not in.

>+  _getParticipant: function(aNick, aNotifyObservers) {
>+    let normalizedNick = normalize(aNick, true);
>+    if (!this._participants.hasOwnProperty(normalizedNick)) {
if (!this._hasParticipant(aNick)) ? Not sure, actually...

>+function ConvChatBuddy(aName) {
>+  // XXX move this outside the function?
>+  const nameModes = {"@": "op", "%": "halfOp", "+": "voiced"};
warning: this may be different on different IRC servers -_-

>+ConvChatBuddy.prototype = {
>+  __proto__: GenericConvChatBuddyPrototype,
>+  _setMode: function(aNewMode) {
it would be nice to document what aNewMode should be

>+    const modes = {"a": "away", // User is flagged as away
>+                   "i": "invisible", // Marks a user as invisible
>+                   "w": "wallop", // User receives wallops
>+                   "r": "restricted", // Restricted user connection
>+                   "o": "op", // Operator flag
>+                   "h": "halfOp", // Half operator flag
>+                   "O": "lop", // Local operator flag
>+                   "s": "server"}; // User receives server notices
>+                   //XXX voice, channel creator? see 4.1 of RFC 2811
warning: this might be different on different servers, too -_-

>+Conversation.prototype = {

>+  sendMsg: function(aMessage) {
>+    this._account._sendMessage("PRIVMSG", [aMessage], this.name);
>+    this.writeMessage(this._account._nickname,
>+                      aMessage,
>+                      {outgoing: true});
same as Chat.sendMsg - comment that the server won't send the message back

>+function ircSocket(aAccount) {
>+  // Implement Section 5 of RFC 2812
>+  this.onDataReceived = (function(aRawMessage) {
...
>+  }).bind(aAccount);
odd that this isn't bound to this (i.e. handleMessage gets aAccount isntead of
a ircSocket above).  Comment please.

>+  this.onConnectionReset = (function () {
>+    // Display the error in the account manager
>+    this.reportDisconnecting(Ci.purpleIAccount.ERROR_NETWORK_ERROR,
>+                            "Connection reset.");
>+    this.reportDisconnected(); // Start the reconnection timer
>+    this._socket.disconnect();
>+    Cu.reportError("Connection reset.");
You probably want to 1) use some other mechanism than Cu.reportError, 2) be
more verbose - connection to what was reset?

>+ircSocket.prototype = {

>+  // Let's keep track of what's going on in the socket
>+  onConnectionTimedOut: function() { Cu.reportError("Timed out"); },
why are these in the proto, and the other methods in the function?  is it so
you can access aAccount (which you can do by stashing it as a member anyway)? 
Something magically wrong with that? (in which case that needs a comment too)

>+function Account(aProtocol, aImAccount) {

>+  let matches = aImAccount.name.split("@", 2); // XXX should this use the username split?
[this._nickname, this._server] = aImAccount.name.split("@", 2);

>+Account.prototype = {

>+  _mode: 0x00, // bit 2 is 'w' (wallops) and bit 3 is 'i' (invisible)
better if you have constants
_MODE_WALLOPS = 1<<2,
_MODE_INVISIBLE = 1<<3,

>+  joinChat: function(aComponents) {
>+    let params = [aComponents.getValue("channel")];
>+    if (aComponents.getValue("password"))
>+      params.push(aComponents.getValue("password"));
doing the get twice looked odd; meh.

>+  _sendMessage: function(aCommand, aParams, aTarget) {
seems odd for aTarget to be after aParams.  Maybe it's just me.

But maybe the fact that the nick is aTarget (for NICK commands) is the odd
thing here...

>+    if (aTarget)
>+      message += " " + aTarget;
any need to sanity check aCommand / aTarget for spaces? (also aParams)

>+    message = message.replace(/&lt;/g, "<").replace(/&gt;/g, ">")
>+                     .replace(/&amp;/g, "&");
add a comment about why this is needed?  (possibly with a bug number, if it's a
bug)

>+    try {
>+      this._socket.sendData(message);
>+    } catch(e) {
>+      this._disconnect();
>+      this.reportDisconnected(Ci.prplIAccount.ERROR_NETWORK_ERROR,
>+                              "Socket closed unexpectedly.")
should |e| be examined for the actual error?

>+    // Send the IRC message as a NOTICE or PRIVMSG.
>+    this._sendMessage(!!isNotice ? "NOTICE" : "PRIVMSG", params, aTarget);
I don't think you need the !! here, the ? will make it bool anyway

>+function Protocol() {
>+  this.registerCommands();
>+
>+  // Register the standard handlers
>+  Cu.import("resource:///modules/ircRfc2812.jsm");
>+  Cu.import("resource:///modules/ircCtcp.jsm");
seems odd to be importing things in the middle of the file (and polluting the
global namespace).

>diff --git a/chat/protocols/irc/ircCommands.jsm b/chat/protocols/irc/ircCommands.jsm

>+function joinCommand(aMsg, aConv) {
>+  if (aMsg.length) {
(this and others) I'd argue for early return,
if (!aMsg.length) return false;

>+    if (params.length > 1)
>+      keys = params[1].split(",");
the way this command is structured makes me cry (no matter what the protocol
says):
  /join channel1,channel2,channel3 key1,,key3

I'd much rather type
  /join channel1,key1 channel2 channel3,key3
(or exchange space and comma, if you prefer?)

>+    for (let i = 0; i < channels.length; i++) {
>+      // XXX verify channel is proper string
>+      params = [channels[i]];
or you can params = [String(channels[1])];
>+
>+      // If a key was given, used it
>+      if (keys.length > i)
>+        params.push(keys[i]);
>+
>+      // params is (<channel>, [<channel>])
I think you mean (<channel>, [<key>])
>+      account._sendMessage("JOIN", params);

>+function kickCommand(aMsg, aConv) {

>+    ircAccounts[aConv.account.id]._sendMessage("KICK", params);
man, I really don't like that ircAccounts global all that much

>+function messageCommand(aMsg, aConv) {
>+  if (aMsg.length) {
>+    let params = aMsg.split(" ");
does this mean the message can't have more than once space? or start with a
space? that sounds broken...

>+function setMode(aMsg, aConv, aMode, aAddMode) {
>+  if (aMsg.length) {
>+    let mode = (!!aAddMode ? "+" : "-") + aMode;
no need for !!, the ? will do it for you, I think

>+var commands = [

>+  {
>+    name: "ctcp",
>+    helpString: "ctcp <nick> <msg>:  Sends ctcp msg to nick.",
>+    run: function(aMsg, aConv)
>+      simpleCommand(aConv, "PRIVMSG", "\001" + aMsg + "\001")
not ctcpMessage?

>+  {
>+    name: "mode",
>+    helpString: "mode &lt;nick|channel&gt; &lt;+|-&gt;&lt;A-Za-z&gt;:  Set " +
why is this one html-escaped, but not the other ones?

>+  // XXX all this does is force the UI to reload the names...which shouldn't be
>+  // out of date
>+  /*{
>+    name: "names",
just call it /refresh and keep it?  always nice to have a way to force that.

>+  {
>+    name: "nick",
>+    helpString: "nick &lt;new nickname&gt;:  Change your nickname.",
also escaped here.
>+    run: function(aMsg, aConv) {
>+      // Ensure the new nick is a valid nickname
>+      if (isNickName(aMsg)) {
>+        ircAccounts[aConv.account.id]._sendMessage("NICK", [aMsg]);
>+        return true;
>+      }
>+      // XXX error message on bad nick?
yeah; start with a hard coded ERROR for now?

>+  {
>+    name: "op",
>+    helpString: "op &lt;nick1&gt; [nick2] ... 	Grant channel operator status to " +
is this slightly too wide? (if you're doing string concats anyway...)

>+  {
>+    name: "part",
can this be aliased to /leave? (though I tend to just /leave <message> without
the channel name...)
>+    helpString: "part [room [message]]:  Leave the current channel, or a " +
>+                "specified channel, with an optional message.",

>+  {
>+    name: "query",
hmm; I can just /que in cz; probably too much to ask for here...

>+  {
>+    name: "quote",
(cz has a /raw alias; I guess that's not really necessary...)

>+  {
>+    name: "version",
>+    helpString: "version [nick]:  Send CTCP VERSION request to a user.",
is the nick optional here?

>+  {
>+    name: "whois",
cz does this nice thing where if the user is offline it automatically runs
/whowas. might want to consider doing that in a follow-up.

>diff --git a/chat/protocols/irc/ircCtcp.jsm b/chat/protocols/irc/ircCtcp.jsm

>+function lowLevelDequote(aString) {
I assume this was changed as we discussed :)

>+// Choose whether to start or stop formatting.
>+// aCurrentFormatting is a string representing the open tags.
but there is no aCurrentFormatting parameter!?  Also, please reword the
documentation to actually make sense, or something...

also, it sounds like this should be a local function inside of ctcpFormat?
>+function applyFormatting(aFormatting, aFormatString) {

>+  if (isOn) {
>+    finalFormatting = aFormatString.slice(0, index) +
>+                      aFormatString.slice(index + 1);
>+  } else
>+    finalFormatting = aFormatString + aFormatting;
is this trying to close a tag? if yes, I _think_ aFormatting hasn't got the /
in it (else the lastIndexOf would have failed)

>+function ctcpFormat(aString) {
>+  // Handle "old style" formatting, which is depracated according to the
"deprecated", I think?

>+  for (i = 0; i < aString.length; i++) {
>+    // For each formatting, alternate whether to start the formatting or end it
>+    // based on how many have been applied.
I think this will be saner if you just kept a {bold, italic, underline} sets of
bools and toggle them as you do things.  no more magic string grepping...

>+var ctcpHandleMessage = function(aMessage) {

>+  if (ctcpHandlers == undefined)
>+    registerCTCPHandler(ctcp);
I hate magic globals :(

>+  // Loop over each raw CTCP message
>+  for each (let message in ctcpMessages) {
>+    Cu.reportError(JSON.stringify(message));
// XXX kill me

>+var ctcp = {

>+  // These represent CTCP commands.
>+  commands: {

>+    // Used when an error needs to be replied with.
>+    "ERRMSG": function(aMessage) false,
no need to log this (as debug)?

>+    // Used to measure the delay of the IRC network between clients.
>+    "PING": function(aMessage) {

>+        // The received timestamp is invalid
>+        if (sentTime == "Invalid Date") {
try if (isNaN(sentTime))?

>+        let response = "Received PING response. There is a delay of " + delay +
>+                       " to " + aMessage.nickname;
That's a little verbose. CZ just has "Ping reply from %nick% in %delay%
seconds." - something around that length is probably enough.

>+    "SED": function(aMessage) false,
would be nice to have a comment about what this crazy thing does :D

>+    // The version and type of the client.
>+    "VERSION": function(aMessage) {

>+        let message = "Received VERSION response: " + aMessage.ctcpParam + ".";
how about just "%nick% is using %version%" or something short like that?

>diff --git a/chat/protocols/irc/ircHandlers.jsm b/chat/protocols/irc/ircHandlers.jsm

>+var EXPORTED_SYMBOLS = ["registerHandler", "unregisterHandler", "handleMessage",
>+                        "registerCTCPHandler",
>+                        "ircHandlers", "ctcpHandlers"];
that's a little more exports than what I normally do (I'd just have one object
with lots of properties), but whatever floats your boat I guess

>+/*function unregisterHandler(aIrcHandlerName) {
ircHandlers.filter(function(h) h.name == aIrcHandlerName)
           .forEach(function(h) ircHandlers.splice(ircHandlers.indexOf(h), 1));
>+}*/

>+ * Object to hold the CTCP specifications, see above for the fields toimplement.
nit: "to implement" (missing space)

>+// Handle a message based on a set of handlers.
>+// 'this' is the JS account object.
but you don't use |this| anywhere in the function?
>+function handleMessage(aAccount, aHandlers, aMessage, aCommand) {

>diff --git a/chat/protocols/irc/ircRfc2812.jsm b/chat/protocols/irc/ircRfc2812.jsm

>+var EXPORTED_SYMBOLS = ["rfc2812"];
this needs a comment describing what rfc 2812 is about.

>+var rfc2812 = {

>+  commands: {

>+    "NICK": function(aMessage) {
>+      // NICK <nickname>
>+      for each (let conversation in this._conversations) {
>+        if (conversation.isChat &&
>+            conversation._hasParticipant(aMessage.nickname)) {

>+        } else {
>+          // XXX need to handle a private conversation also.
file follow up and note bug number here?

>diff --git a/chat/protocols/irc/ircUtils.jsm b/chat/protocols/irc/ircUtils.jsm

>+// This can be used to test is a string is a valid nickname string
>+function isNickName(aStr) {
>+  return /[A-Za-z\[\]\\`_^\{\|\}][A-Za-z0-9\-\[\]\\`_^\{\|\}]*/.test(normalize(aStr))
I believe _some_ servers can have... interesting nicknames. (Try one of the
ones using gb2312 as the code page?)

Comment 18

[Florian Quèze [:florian]]

*** Original post on bio 507 at 2011-12-03 09:11:16 UTC ***

(In reply to comment #11)
> Drive-by review;

Thanks, this is definitely very appreciated! :)

> >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> 
> >+function rfc2812Message(aData) {
> >+  LOG(aData);
> >+  let message = {rawMessage: aData};
> >+  let temp;
> >+
> >+  // Splits the raw string into four parts (the second is required)
> >+  //   source
> >+  //   command
> >+  //   [parameter]
> >+  //   [:last paramter]
> >+  // See http://joshualuckers.nl/2010/01/10/regular-expression-to-match-raw-irc-messages/
> >+  // Should be equivalent to the slightly simplier:
> >+  //   /^(?:[:@](\S+) )?(\S+)(?: ((?:[^: ]\S* ?)*))?(?: ?:(.*))?$/
> >+  if ((temp = aData.match(/^(?:[:@]([^ ]+) )?([^ ]+)(?: ((?:[^: ][^ ]* ?)*))?(?: ?:(.*))?$/))) {
> this is probably clearer as two statements:
> 
>   let temp = aData.match(/..../);
>   if (temp) {
>     ...
> speaking as somebody who doesn't really speak IRC: even better if this isn't an
> RE, or split into several, or a more explicit BNF in the comment.

What's the problem with the RE? Is it only readability (if so, the comment could probably be significantly improved with an example of input with an ascii art indication of which part is what), or is there something else?
A regexp sounds like a quite efficient way to do this to me. Parsing with lots of split/indexOf/lastIndexOf calls seems quite confusing and error prone to me.

> >+  unInit: function() {
> >+    if (this._account.connectionState == Ci.purpleIAccount.STATE_CONNECTED)
> >+      this._account._sendMessage("PART", [this.name]);
> I hope the server can deal with people PARTing channels they're not in.

Adding a test to check if we have already left the room doesn't sound difficult.

> >+  this.onConnectionReset = (function () {
> >+    // Display the error in the account manager
> >+    this.reportDisconnecting(Ci.purpleIAccount.ERROR_NETWORK_ERROR,
> >+                            "Connection reset.");
> >+    this.reportDisconnected(); // Start the reconnection timer
> >+    this._socket.disconnect();
> >+    Cu.reportError("Connection reset.");
> You probably want to 1) use some other mechanism than Cu.reportError, 2) be
> more verbose - connection to what was reset?

Either send to the console a message that is a little bit more descriptibe, using WARN/ERROR which will also put location information in the console; or just remove this line, as the error is already being reported to the account manager anyway.


> >+  _mode: 0x00, // bit 2 is 'w' (wallops) and bit 3 is 'i' (invisible)
> better if you have constants
> _MODE_WALLOPS = 1<<2,
> _MODE_INVISIBLE = 1<<3,

I think you meant to type ":" instead of " =" on these 2 lines, but Patrick has probably already guessed that :).

> >+    message = message.replace(/&lt;/g, "<").replace(/&gt;/g, ">")
> >+                     .replace(/&amp;/g, "&");
> add a comment about why this is needed?  (possibly with a bug number, if it's a
> bug)

I changed the core code yesterday as part of my JS-XMPP work so that this isn't needed any more.

> 
> >+    try {
> >+      this._socket.sendData(message);
> >+    } catch(e) {
> >+      this._disconnect();
> >+      this.reportDisconnected(Ci.prplIAccount.ERROR_NETWORK_ERROR,
> >+                              "Socket closed unexpectedly.")
> should |e| be examined for the actual error?

At least Cu.reportError it so that it's possible to see the actual error somewhere?

> >diff --git a/chat/protocols/irc/ircCommands.jsm b/chat/protocols/irc/ircCommands.jsm
> 
> >+function joinCommand(aMsg, aConv) {
> >+  if (aMsg.length) {
> (this and others) I'd argue for early return,
> if (!aMsg.length) return false;

I also asked for that change, it's probably even already in the new attachment. :)

 
> >+    if (params.length > 1)
> >+      keys = params[1].split(",");
> the way this command is structured makes me cry (no matter what the protocol
> says):
>   /join channel1,channel2,channel3 key1,,key3
> 
> I'd much rather type
>   /join channel1,key1 channel2 channel3,key3
> (or exchange space and comma, if you prefer?)

I don't care too much either way. If it was just me, the command would only allow joining a single room at once, and would be /join channel key
But now that you already handle joining several at once, I don't see a reason to simplify...


> >+    ircAccounts[aConv.account.id]._sendMessage("KICK", params);
> man, I really don't like that ircAccounts global all that much

+1!

We should definitely help you to find a good replacement for that!


> >+  {
> >+    name: "part",
> can this be aliased to /leave? (though I tend to just /leave <message> without
> the channel name...)
> >+    helpString: "part [room [message]]:  Leave the current channel, or a " +

Isn't the syntax rather |part [room] [message]|? (that's what libpurple shows in its help message)


> >+  {
> >+    name: "quote",
> (cz has a /raw alias; I guess that's not really necessary...)

We already have a /raw command that skips HTML escaping, so that it's possible to type HTML code in the textbox. I keep wondering if it should be #ifdef DEBUG though.


> >+        // The received timestamp is invalid
> >+        if (sentTime == "Invalid Date") {
> try if (isNaN(sentTime))?

Thanks! I was looking for something similar for my JS-XMPP work. The test on "Invalid Date" seemed fragile, and the Date object is a bit magical, but the Date doc on MDC doesn't mention any recommended way to check for validity of the date contained by a Date instance.


> >+    "SED": function(aMessage) false,

By the way, do you really need all these double quotes around command names?


> >+        let message = "Received VERSION response: " + aMessage.ctcpParam + ".";
> how about just "%nick% is using %version%" or something short like that?

+1 for replacing the user facing messages containing technical gibberish with user readable messages :).

By the way, I think working on localizability is a nice opportunity to review all the user facing messages at once, as the localizable strings are clearly separated from all the other debug messages that we have in the code: it's easier to notice when something nonsensical is likely to be put in the user's face.

> >diff --git a/chat/protocols/irc/ircHandlers.jsm b/chat/protocols/irc/ircHandlers.jsm
> 
> >+var EXPORTED_SYMBOLS = ["registerHandler", "unregisterHandler", "handleMessage",
> >+                        "registerCTCPHandler",
> >+                        "ircHandlers", "ctcpHandlers"];
> that's a little more exports than what I normally do (I'd just have one object
> with lots of properties), but whatever floats your boat I guess

I agree that this module exports too many symbols, that could likely all go inside an IRCHandlers exported object.


> >diff --git a/chat/protocols/irc/ircRfc2812.jsm b/chat/protocols/irc/ircRfc2812.jsm
> 
> >+var EXPORTED_SYMBOLS = ["rfc2812"];
> this needs a comment describing what rfc 2812 is about.

Or even better: no comment, but a name that most developers can understand the first time they read it.


> 
> >+var rfc2812 = {
> 
> >+  commands: {
> 
> >+    "NICK": function(aMessage) {
> >+      // NICK <nickname>
> >+      for each (let conversation in this._conversations) {
> >+        if (conversation.isChat &&
> >+            conversation._hasParticipant(aMessage.nickname)) {
> 
> >+        } else {
> >+          // XXX need to handle a private conversation also.
> file follow up and note bug number here?

Please also note in the comment what's visibly broken for the user until this is addressed. Is it just that the next private message received from that user will appear in a different conversation instead of changing the title of the current conversation?

Of course, if just fixing it is faster than improving the comment/filing a bug, go ahead :).

Comment 19

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2011-12-03 18:09:58 UTC ***

(In reply to comment #11)
> Drive-by review; remember that all I can do is pick nits, since I don't know
> the code well.  Also, my eyes kinda glazed over around all the numeric codes.
> I'm just suggesting style changes; please feel free to ignore anything you
> disagree with.
Thanks for looking over this! Some of the changes have already been made in the newer patch. I'm not going to reply to any of the easy changes that I fully agree with you on just to slim down the comment.

Some of these I waited to read Florian's review first, so that'll be in a separate comment.

> >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> >+  // Splits the raw string into four parts (the second is required)
...
> >+  // See http://joshualuckers.nl/2010/01/10/regular-expression-to-match-raw-irc-messages/
> >+  // Should be equivalent to the slightly simplier:
> this is probably clearer as two statements:
>
>   let temp = aData.match(/..../);
>   if (temp) {
>     ...
> speaking as somebody who doesn't really speak IRC: even better if this isn't an
> RE, or split into several, or a more explicit BNF in the comment.
I expanded the comment here, I don't want to do a lot of indexOfs, etc. it's more confusing (as Florian suggests). I actually used to have what you suggest and we changed it to a regexp: http://hg.instantbird.org/experiments/rev/4aeb5266b881

> >+  sendMsg: function(aMessage) {
> >+    // Only send message if we're in the room
> >+    // XXX is this the expected behavior?
> I believe some IRC bouncers will confuse you here - they prefix your nick with
> something-or-other (and I can't remember what), so it looks like you're not in
> the room (because what you think your nick is isn't there).
>
> Silver (from chatzilla) dislikes those bouncers because of all the
> spec-breaking they do, of course :p
>
> >+    if (this._hasParticipant(this._account._nickname)) {
> >+      this._account._sendMessage("PRIVMSG", [aMessage], this.name);
> >+      this.writeMessage(this._account._nickname, aMessage, {outgoing: true});
> add a comment that this is needed because the server won't send the message
> back to you. (and if you change the participant check before, move it here.)
I'm agreeing with Silver here then. :( I moved the if-statement for now.

> >+  _mode: 0x00, // bit 2 is 'w' (wallops) and bit 3 is 'i' (invisible)
> better if you have constants
> _MODE_WALLOPS = 1<<2,
> _MODE_INVISIBLE = 1<<3,
These aren't used anyway right now. :(, but good change.

> >+  joinChat: function(aComponents) {
> >+    let params = [aComponents.getValue("channel")];
> >+    if (aComponents.getValue("password"))
> >+      params.push(aComponents.getValue("password"));
> doing the get twice looked odd; meh.
I agree that this looks odd, it's the terrible API we (libpurple) have around getting information from the join chat menu.


> >+function Protocol() {
> >+  this.registerCommands();
> >+
> >+  // Register the standard handlers
> >+  Cu.import("resource:///modules/ircRfc2812.jsm");
> >+  Cu.import("resource:///modules/ircCtcp.jsm");
> seems odd to be importing things in the middle of the file (and polluting the
> global namespace).
I'm loading them into a scope now and then registering, I think this is adequate and shouldn't have issues. Thanks for catching this!


> >+function messageCommand(aMsg, aConv) {
> >+  if (aMsg.length) {
> >+    let params = aMsg.split(" ");
> does this mean the message can't have more than once space? or start with a
> space? that sounds broken...
Ah, this could use the Python split you were discussing! Instead I replaced params[1] with params.slice(1).join(" "), but perhaps I should use indexOf instead.

> >+  // XXX all this does is force the UI to reload the names...which shouldn't be
> >+  // out of date
> >+  /*{
> >+    name: "names",
> just call it /refresh and keep it?  always nice to have a way to force that.
Maybe, but I'd rather only add it if it's necessary.

> >+  {
> >+    name: "part",
> can this be aliased to /leave? (though I tend to just /leave <message> without
> the channel name...)
After this lands, please file a bug requesting aliases for the CZ command names (/que, etc.) and I'll add them (or make a CZ compatibility extension). I'd rather not worry about this kind of thing now.

> >+    name: "version",
> >+    helpString: "version [nick]:  Send CTCP VERSION request to a user.",
> is the nick optional here?
libpurple seems to be confused about the difference between < > and [ ] and ( ) in these descriptions. At least the ones I usually use.

> >+    name: "whois",
> cz does this nice thing where if the user is offline it automatically runs
> /whowas. might want to consider doing that in a follow-up.
That sounds like a great idea! (And I can alias /whois and /whowas to the same command). Let's worry about this in a different bug.

> >+function lowLevelDequote(aString) {
> I assume this was changed as we discussed :)
Yes. Thanks! :)

> >+  for (i = 0; i < aString.length; i++) {
> >+    // For each formatting, alternate whether to start the formatting or end it
> >+    // based on how many have been applied.
> I think this will be saner if you just kept a {bold, italic, underline} sets of
> bools and toggle them as you do things.  no more magic string grepping...
The reason I did it this was the was in this comment:
>+      // Close the tags in the opposite order they were opened.
I originally had separate booleans and I like that better. I need to think about this more to not make really invalid HTML.

> >+  if (ctcpHandlers == undefined)
> >+    registerCTCPHandler(ctcp);
> I hate magic globals :(
registerCTCPHandler or ctcp? ctcp is defined in this file and registerCTCPHandler is from ircHandlers.jsm. You'd prefer a single exported object there though?

> >+    "SED": function(aMessage) false,
> would be nice to have a comment about what this crazy thing does :D
I agree, this is about all the information I have on it (from the spec):
> [SED is a simple encryption protocol between IRC clients implemented with CTCP. I don't have any reference for it -- Ben]

Thanks again!

Comment 20

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2011-12-03 18:11:02 UTC ***

A few more response from Florian's comments.

> > >+  unInit: function() {
> > >+    if (this._account.connectionState == Ci.purpleIAccount.STATE_CONNECTED)
> > >+      this._account._sendMessage("PART", [this.name]);
> > I hope the server can deal with people PARTing channels they're not in.
> Adding a test to check if we have already left the room doesn't sound
> difficult.
Done as suggested! Needs to be tested, however.

> > >+    if (params.length > 1)
> > >+      keys = params[1].split(",");
> > the way this command is structured makes me cry (no matter what the protocol
> > says):
> >   /join channel1,channel2,channel3 key1,,key3
> >
> > I'd much rather type
> >   /join channel1,key1 channel2 channel3,key3
> > (or exchange space and comma, if you prefer?)
>
> I don't care too much either way. If it was just me, the command would only
> allow joining a single room at once, and would be /join channel key
> But now that you already handle joining several at once, I don't see a reason
> to simplify...
This (and most other commands) are structured as libpurple have them. I'm not married to any of them but I needed SOMETHING to put in. I'd like to handle the exact form of the commands in a different bug if possible? I see bikeshedding from it.

> > >+    ircAccounts[aConv.account.id]._sendMessage("KICK", params);
> > man, I really don't like that ircAccounts global all that much
>
> +1!
>
> We should definitely help you to find a good replacement for that!
I hate it as well. I'm going to try some of the things Mook suggested on IRC last night to try to get rid of it.

> > >+    helpString: "part [room [message]]:  Leave the current channel, or a " +
> Isn't the syntax rather |part [room] [message]|? (that's what libpurple shows
> in its help message)
No, you can't give a message without a channel first. (So the message is only optional if you've first given a channel). But maybe we should just have it as /part [message].

> > >+    "SED": function(aMessage) false,
> By the way, do you really need all these double quotes around command names?
Probably not, I wasn't sure if you could use numbers here (and how that would handle situations like 001), so I quoted those and then quoted all of them for conformity.

> > >+var EXPORTED_SYMBOLS = ["rfc2812"];
> > this needs a comment describing what rfc 2812 is about.
>
> Or even better: no comment, but a name that most developers can understand the
> first time they read it.
I agree, but I wasn't sure what to name it. :( Any suggestions would be appreciated. (It's the base protocol described in RFC 2812 :P).

Comment 21

[Florian Quèze [:florian]]

*** Original post on bio 507 at 2011-12-03 19:01:55 UTC ***

(In reply to comment #13)

> > >+    let params = [aComponents.getValue("channel")];
> > >+    if (aComponents.getValue("password"))
> > >+      params.push(aComponents.getValue("password"));
> > doing the get twice looked odd; meh.
> I agree that this looks odd, it's the terrible API we (libpurple) have around
> getting information from the join chat menu.

I think he meant you need a "password" intermediate variable here:

    let password = aComponents.getValue("password");
    if (password)
      ...

(In reply to comment #14)

> > > >+    helpString: "part [room [message]]:  Leave the current channel, or a " +
> > Isn't the syntax rather |part [room] [message]|? (that's what libpurple shows
> > in its help message)
> No, you can't give a message without a channel first. (So the message is only
> optional if you've first given a channel).

Right, the libpurple help message is wrong then.

> But maybe we should just have it as
> /part [message].

Fine with me! :)

> > > >+var EXPORTED_SYMBOLS = ["rfc2812"];
> > > this needs a comment describing what rfc 2812 is about.
> >
> > Or even better: no comment, but a name that most developers can understand the
> > first time they read it.
> I agree, but I wasn't sure what to name it. :( Any suggestions would be
> appreciated. (It's the base protocol described in RFC 2812 :P).

IRC or IRCBase

Comment 22

[Patrick Cloke [:clokep]]

Attachment: WIP 4

*** Original post on bio 507 as attmnt 1050 at 2011-12-04 15:36:00 UTC ***

This makes the fixes discussed in the previous review from Mook & Florian (and includes the changes from comment #15). This also includes some l10n (although I have not done this for ircCommands.jsm yet, which has a lot of user facing strings).

I'm going to start working on some of the bigger changes Mook suggested (including trying to find a replacement for the dreaded ircAccounts global!)

Comment 23

[Patrick Cloke [:clokep]]

Comment on attachment 8352789 [details] [diff] [review]
WIP 3

*** Original change on bio 507 attmnt 1047 at 2011-12-04 15:36:41 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 24

[Patrick Cloke [:clokep]]

Attachment: WIP 5

*** Original post on bio 507 as attmnt 1051 at 2011-12-04 20:58:00 UTC ***

Now exports an ircHandlers object which gives a real API for someone to extend the IRC protocol implementation (also increases code reuse and simplifies a lot of other code). Thanks Mook!
It should also be able to unregister handlers thanks to Mook's code.

> >+function ircSocket(aAccount) {
> >+  // Implement Section 5 of RFC 2812
> >+  this.onDataReceived = (function(aRawMessage) {
> ...
> >+  }).bind(aAccount);
> odd that this isn't bound to this (i.e. handleMessage gets aAccount isntead of
> a ircSocket above).  Comment please.
>
> >+  // Let's keep track of what's going on in the socket
> >+  onConnectionTimedOut: function() { Cu.reportError("Timed out"); },
> why are these in the proto, and the other methods in the function?  is it so
> you can access aAccount (which you can do by stashing it as a member anyway)?
> Something magically wrong with that? (in which case that needs a comment too)
I got rid of all the binds, I did this originally to avoid the (very easy) rewrite of the methods.

> >+  sendMessage: function(aCommand, aParams, aTarget) {
> seems odd for aTarget to be after aParams.  Maybe it's just me.
>
> But maybe the fact that the nick is aTarget (for NICK commands) is the odd
> thing here...
The target field was kind of a hack so that the NICK and PASS commands can send their parameters without being prefixed with a colon. This is apparently not necessary and you CAN prefix them with colons, which let's me treat them as all other commands. I've removed the target: it's really just the first parameter anyway.

l10n is complete (to my knowledge, it's possible I missed a string, it's hard to find strings :().

ircCommands.jsm was cleaned up significantly.

Comment 25

[Patrick Cloke [:clokep]]

Comment on attachment 8352792 [details] [diff] [review]
WIP 4

*** Original change on bio 507 attmnt 1050 at 2011-12-04 20:58:15 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 26

[Patrick Cloke [:clokep]]

Attachment: WIP 6

*** Original post on bio 507 as attmnt 1052 at 2011-12-05 03:28:00 UTC ***

Did a bit more work on this today.

Use getters for command localizations.

Removed ircAccounts global object, the GenericConversationPrototype now has a wrappedJSObject available.

Displays CTCP formatting (bold, italic and underline) in messages, strips them for topics.

7:23:19 PM - flo:  this.sendMessage("AWAY", [aMsg || "I am away from my computer."]);
7:23:25 PM - flo: why is there an hard coded message here?
It seems that you have to give a message to actually mark yourself as away, if no message is given you're marked as "back":
   The AWAY command is used either with one parameter, to set an AWAY
   message, or with no parameters, to remove the AWAY message.
This is now handled as we discussed over IRC. I'm using the default idle message from the preference.

Observes Instantbird status changes.

Working VERSION command. I need to check more of the CTCP commands. But the basic structure works.

The big thing left to do is to handle buddies and interacting with the buddy list.

Comment 27

[Patrick Cloke [:clokep]]

Comment on attachment 8352793 [details] [diff] [review]
WIP 5

*** Original change on bio 507 attmnt 1051 at 2011-12-05 03:28:40 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 28

[Instantbot]

*** Original post on bio 507 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-12-05 05:55:13 UTC ***

(In reply to comment #12)
> (In reply to comment #11)

> > >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> > 
> > >+function rfc2812Message(aData) {

> > speaking as somebody who doesn't really speak IRC: even better if this isn't an
> > RE, or split into several, or a more explicit BNF in the comment.
> 
> What's the problem with the RE? Is it only readability (if so, the comment
> could probably be significantly improved with an example of input with an ascii
> art indication of which part is what), or is there something else?
> A regexp sounds like a quite efficient way to do this to me. Parsing with lots
> of split/indexOf/lastIndexOf calls seems quite confusing and error prone to me.

Yeah, just the readability issues.

> > >+        // The received timestamp is invalid
> > >+        if (sentTime == "Invalid Date") {
> > try if (isNaN(sentTime))?
> 
> Thanks! I was looking for something similar for my JS-XMPP work. The test on
> "Invalid Date" seemed fragile, and the Date object is a bit magical, but the
> Date doc on MDC doesn't mention any recommended way to check for validity of
> the date contained by a Date instance.

Yeah, this is sort of a hack - isNaN tries to convert its param to a double; in
the Date case (and, I think, probably everything else) this happens via .valueOf(),
which gives NaN for invalid things.

(In reply to comment #13)
> (In reply to comment #11)

> Thanks for looking over this! Some of the changes have already been made in the
> newer patch. I'm not going to reply to any of the easy changes that I fully
> agree with you on just to slim down the comment.

Yep, right as I submitted bugzilla told me I had a collision with you attaching a new patch... I figured I should submit anyway :p

> > >+  {
> > >+    name: "part",
> > can this be aliased to /leave? (though I tend to just /leave <message> without
> > the channel name...)
> After this lands, please file a bug requesting aliases for the CZ command names
> (/que, etc.) and I'll add them (or make a CZ compatibility extension). I'd
> rather not worry about this kind of thing now.
Sounds reasonable :)  Please don't block landing on any of my comments (unless you feel like they should) :D

> > >+  for (i = 0; i < aString.length; i++) {
> > >+    // For each formatting, alternate whether to start the formatting or end it
> > >+    // based on how many have been applied.
> > I think this will be saner if you just kept a {bold, italic, underline} sets of
> > bools and toggle them as you do things.  no more magic string grepping...
> The reason I did it this was the was in this comment:
> >+      // Close the tags in the opposite order they were opened.
> I originally had separate booleans and I like that better. I need to think
> about this more to not make really invalid HTML.
Ah, yes, that makes things annoying, especially when you do silly things like
  ^B bold ^U bold underline ^B underline only ^U nothing
(The HTML parser will deal with invalid markup in this case, but we should still not emit broken markup and rely on the parser fixing us.)

Something like this?  Whether this is _actually_ clearer is something you want to confirm, though - just trying to implement a naive algorithm.

  /**
   * Convert a string from CTCP escaped formatting to HTML markup
   * @param aString the string with CTCP formatting to parse
   * @return The HTML formatted string
   */
  function ctcpFormat(aString) {
    let next, stack = [], input = aString, output = "";
    while ((next = /\^[\x02|\x16|\x1F|\x0F]/.exec(input))) {
      if (next.index > 0)
        output += input.substr(0, next.index - 1);
      let tag = {"\x02": "b", // \002, ^B
                 "\x16": "i", // \026, ^V 
                 "\x1F": "u", // \037, ^_
                 "\x0F": "O", // \017, ^O
                }[input[next.index + 1]];
      if (tag == "O") {
        // clear all formatting
        stack.reverse().forEach(function(tag) output += "</" + tag + ">");
        stack = [];
      } else {
        let offset = stack.indexOf(tag);
        if (offset < 0) {
          // not found; open new tag
          output += "<" + tag + ">";
          stack.push(tag);
        } else {
          // found; close existing tag
          stack.slice(offset + 1).reverse()
               .forEach(function(tag) output += "</" + tag + ">");
          output += "</" + tag + ">";
          stack.slice(offset + 1)
               .forEach(function(tag) output += "<" + tag + ">");
          stack.splice(offset, 1);
        }
      }
      input = input.substr(next.index + 2); // I'm using ^foo for now
    }
    return output + input; // return unmatched bits too
  }


I'll go look at the updated patch soon.

Comment 29

[Instantbot]

*** Original post on bio 507 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-12-05 07:56:52 UTC ***

Comment on attachment 8352794 [details] [diff] [review] (bio-attmnt 1052)
WIP 6

(from review of attachment 8352794 [details] [diff] [review] (bio-attmnt 1052))
Again: please feel free to ignore comments that you don't think needs addressing; that's the nature of drive-bys :)  I'm just doing a brain dump as I read.

>diff --git a/chat/locales/en-US/irc.properties b/chat/locales/en-US/irc.properties

>+ctcp.ping=Ping reply from %S in %S seconds.
Style nit: for multiple replacements try using %1$S and %2$S explicitly (even when they're in that order).  Makes things clearer.

>diff --git a/chat/locales/jar.mn b/chat/locales/jar.mn

>+  locale/@AB_CD@/purple/irc.properties (%irc.properties)
nit: all the other lines start with tabs, this one starts with a space

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js

>+XPCOMUtils.defineLazyGetter(this, "_ib", function()
>+  l10nHelper("chrome://instantbird/locale/instantbird.properties")
style nit: semicolon :p (unless you're in the prefer automatic insertion camp, in which case please ignore me.)

>+function rfc2812Message(aData) {

>+  //   [":" <source> " "] <command> [" " <parameter>]* [":" <last parameter>]
I think it should start with "[:@]" or something along those lines? that's what the RE seems to say

>+Chat.prototype = {

>+  // Overwrite the writeMessage function to apply CTCP formatting before
>+  // display.
>+  // Overwrite the writeMessage function to apply CTCP formatting before
>+  // display.
you're repeating ;)
>+  writeMessage: function(aWho, aText, aProperties) {

>+  unInit: function() {
>+    // Tell the server about the part if connected & if we haven't parted yet.
>+    if (this._account.connected &&
>+        this._hasParticipant(this._account._nickname))
>+      this._account.sendMessage("PART", this.name);
this time it means you can't part a channel if you don't seem to be listed in it :p
(see the bouncer example from before...)

>+  _updateNick: function(aOldNick, aNewNick) {
>+    // Get the original ConvChatBuddy and then remove it
>+    let convChatBuddy = this._getParticipant(aOldNick);
if aOldNick isn't found for some reason, observers will see chat-buddy-update without a chat-buddy-add before. does this matter?

>+  get left() this._hasParticipant(this.name),
shouldn't this be inverted? (assuming this property means "have I left the chat?")

>+ConvChatBuddy.prototype = {

>+  _setMode: function(aNewMode) {

>+    // Are we going to add or remove the modes?
>+    let newMode = (aNewMode[0] == "+") ? true : false;
>+    if (aNewMode[0] != "-") {
that seems wrong (when newMode == "+foo" for example)
>+      ERROR("Invalid mode string: " + aNewMode);
>+      return;
>+    }

>+Account.prototype = {

>+  get _mode() this._MODE_WALLOPS + this._MODE_INVISIBLE,
accounts always have invisible wallops? (reading more things below: this appears
to be used as a constant for now, so I guess this is fine.)

>+  _setStatus: function() {

>+    const DEFAULT_IDLE_PREF = "messenger.status.defaultIdleAwayMessage";
>+    if (!text && Services.prefs.prefHasUserValue(DEFAULT_IDLE_PREF))
>+      text = Services.prefs.getCharPref(DEFAULT_IDLE_PREF);
>+    else
>+      text = _ib("messenger.status.defaultIdleAwayMessage");
this means you're override text if this.imAccount.statusInfo.statusText is not blank?

you probably wnat
if (!text && Services.prefs.getPrefType(DEFAULT_IDLE_PREF) == Services.prefs.PREF_STRING)
  text = Services.prefs.getCharPref(DEFAULT_IDLE_PREF);
if (!text)
  text = _ib("messenger.status.defaultIdleAwayMessage");

>+  _getConversation: function(aConversationName) {
>+    let normalizedName = normalize(aConversationName);
>+    if (!this._hasConversation(aConversationName)) {
>+      // Handle Scandanavian lower case
this comment look pretty out of place now?
>+      let constructor = isMUCName(normalizedName) ? Chat : Conversation;
>+      this._conversations[normalizedName] =
>+        new constructor(this, aConversationName, this._nickname);
>+    }

>+  sendMessage: function(aCommand, aParams) {

>+    if (params.length) {

>+      // Join the parameters with spaces, except the last parameter which gets
>+      // joined with a " :" before it (and can contain spaces).
>+      message += " " + params.concat(params, ":" + params.pop()).join(" ");
should this be params.concat (instance method) or Array.concat (class static method)?

>+  _sendCTCPMessage: function(aCommand, aParams, aTarget, isNotice) {
this has a bunch of debug logging that needs to die :)

>+  // Implement section 3.1 of RFC 2812
"Implement section 3.1 of RFC 2812, "Connection Registration""?
>+  _connectionRegistration: function() {

>+function Protocol() {
>+  this.registerCommands();
>+
>+  // Register the standard handlers
>+  let tempScope = {};
>+  Cu.import("resource:///modules/ircBase.jsm", tempScope);
this is good too; no need to change, but I just decided that my personal style in the future is:
  let { ircBase } = Cu.import("...", {});

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm

>+function serverMessage(aMessage) {

>+function serverMessageEnd(aMessage) {
what's the difference between serverMessage and serverMessageEnd? If the only difference is dropping the last arg, sounds like it can be one function with an optional param...

>+var ircBase = {

>+    "JOIN": function(aMessage) {

>+          let joinMessage = aMessage.nickname + " [<i>" + aMessage.source +
>+                            "</i>] entered the room.";
l10n
>+          conversation.writeMessage(aMessage.nickname,
>+                                    joinMessage,
>+                                    {system: true});

>+    "KICK": function(aMessage) {

>+        for each (let username in usersNames) {
>+          let kickMessage = username + " has been kicked";
l10n (below, too, where it gets modified based on aMessage.params[2])

>+    "MODE": function(aMessage) {

>+        let modeMessage = "mode (" + aMessage.params[1] + " " +
l10n... you get the idea :p (I'm ignoring it for the rest of the incoming commands)

>+    "TOPIC": function(aMessage) {

>+      this._getConversation(aMessage.params[0]).writeMessage(
>+        aMessage.nickname || aMessage.source,
>+        aMessage.nickname + " has changed the topic to: " + aMessage.params[1],
put aMessage.nickname || aMessage.source into a temporary, since you want it for the text too

>diff --git a/chat/protocols/irc/ircCommands.jsm b/chat/protocols/irc/ircCommands.jsm

>+var commands = [

>+    name: "ctcp",
>+    get helpString() _("command.ctcp", "ctcp"),
>+    // XXX this looks wrong, it needs to account for the <nick>
>+    // XXX Can this use ctcpCommand?
but it does use ctcpCommand?

>+      return ctcpCommand(aConv, aMsg.slice(0, separator),

>diff --git a/chat/protocols/irc/ircCtcp.jsm b/chat/protocols/irc/ircCtcp.jsm

>+function CTCPMessage(aMessage, aRawCTCPMessage) {

>+  if (separator == -1) {
>+    message.ctcpCommand = dequotedCTCPMessage;
>+    message.ctcpParam = [];
this is an array, and not a string? (doesn't match the other branch)
>+  } else {
>+    message.ctcpCommand = dequotedCTCPMessage.slice(0, separator);
>+    message.ctcpParam = dequotedCTCPMessage.slice(separator + 1);
>+  }

>+function ctcpHandleMessage(aMessage) {

>+  while ((temp = /^\x01([\w\W]*)\x01([\w\W])*$/.exec(ctcpRawMessage))) {
the second capture looks wrong ([\w\W])* vs ([\w\W]*)
actually, I think I've got a better way to do this :D

  otherMessage = ctcpRawMessage.replace(/\x01([^\x01]*)\x01/g, function(match, msg) {
    if (msg)
      ctcpMessages.push(new CTCPMessage(aMessage, msg));
    return "";
  }

Comment 30

[Florian Quèze [:florian]]

*** Original post on bio 507 at 2011-12-05 10:12:45 UTC ***

(In reply to comment #20)
> Comment on attachment 8352794 [details] [diff] [review] (bio-attmnt 1052) [details]

> >+    const DEFAULT_IDLE_PREF = "messenger.status.defaultIdleAwayMessage";
> >+    if (!text && Services.prefs.prefHasUserValue(DEFAULT_IDLE_PREF))
> >+      text = Services.prefs.getCharPref(DEFAULT_IDLE_PREF);
> >+    else
> >+      text = _ib("messenger.status.defaultIdleAwayMessage");
> this means you're override text if this.imAccount.statusInfo.statusText is not
> blank?
> 
> you probably wnat
> if (!text && Services.prefs.getPrefType(DEFAULT_IDLE_PREF) ==
> Services.prefs.PREF_STRING)
>   text = Services.prefs.getCharPref(DEFAULT_IDLE_PREF);
> if (!text)
>   text = _ib("messenger.status.defaultIdleAwayMessage");

Please use getComplexValue (and stop that funny business with that poor pref :-P), see http://lxr.instantbird.org/instantbird/source/chat/components/src/imCore.js#173 for an example.

I also don't think you need to touch the instantbird.properties bundle directly in prpl code, and I really hope you don't, as that would be a problem for reuse of chat/ in another application (Thunderbird? ;)).

By the way, if the user has edited that preference and replaced it with an empty string, shouldn't you return early?

Comment 31

[Patrick Cloke [:clokep]]

Attachment: WIP 7

*** Original post on bio 507 as attmnt 1077 at 2011-12-12 02:37:00 UTC ***

This version has the following changes:
 - Properly get the default away message string.
 - Used Mook's code for CTCP formatting --> HTML (thanks again!).
 - Finished l10n for real (maybe)? I'm sure the strings can use a lot of work (they're probably too technical still), feedback welcome!
 - Misc. clean up of ircBase.jsm & Mook's comments.
 - Use Component.utils.reportError instead of ERROR inside of a try-catch to get real line #s.
 - Buddy support (using ISON polling at 1 minute intervals, at each interval a new message is sent that iterates through the buddies).
 - Changes some terminology to be IRC specific (e.g. referring to "Channel" instead of "Chat" in the code).

I still have a TODO list, which seems to be getting longer, rather than shorter:
 - Get "left" to work for MUCs.
 - Use sendString for the socket (and ensure it works!) / receive non-ASCII messsages
 - Handle who(i|wa)s responses & tooltips
 - Check that ALL commands work
 - A couple miscellaneous comments from above.

Comment 32

[Patrick Cloke [:clokep]]

Comment on attachment 8352794 [details] [diff] [review]
WIP 6

*** Original change on bio 507 attmnt 1052 at 2011-12-12 02:37:32 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 33

[Instantbot]

*** Original post on bio 507 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2011-12-15 08:34:14 UTC ***

Comment on attachment 8352819 [details] [diff] [review] (bio-attmnt 1077)
WIP 7

(from review of attachment 8352819 [details] [diff] [review] (bio-attmnt 1077))

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js

>+function rfc2812Message(aData) {

>+  sendMsg: function(aMessage) {
>+      this._account.sendMessage("PRIVMSG", [this.name, aMessage]);
indentation here looks odd
>+
>+    // Since we don't receive a message back from the server, just assume it

>diff --git a/chat/protocols/irc/ircCommands.jsm b/chat/protocols/irc/ircCommands.jsm

>+// This will send a command to the server, if no message is given, it is assumed
>+// that the command takes no parameters. aMsg can be either a single string or
what aMsg?
>+// an array of parameters.
>+function simpleCommand(aConv, aCommand, aParams) {

>diff --git a/chat/protocols/irc/ircCtcp.jsm b/chat/protocols/irc/ircCtcp.jsm

>+const VERSION = "Instantbird (JS-IRC)";
RFE for much, much later: make this customizable (possibly via prefs)? Though I guess I can override the handler already...

>+// Split into a CTCP message which is a single command and a single parameter:
>+//   <command> " " <parameter>
>+// The high level dequote to unescape \001 in the message content.
s/^The/Then/? Otherwise I can't parse the sentence.
>+function CTCPMessage(aMessage, aRawCTCPMessage) {

>+function ctcpHandleMessage(aMessage) {

>+  // The raw CTCP message is in the last parameter of the IRC message.
>+  let ctcpRawMessage = lowLevelDequote(aMessage.params.slice(-1)[0]);
heh, I had trouble reading that param. But my alternatives are probably not more readable...
 aMessage.params.concat().pop()
 aMessage.params[aMessage.params.length - 1]
 aMessage.params.reverse()[0]
Makes me appreciate python, aMessage.params[-1]

>+  let otherMessage = ctcpRawMessage.replace(/\x01([^\x01]*)\x01/g, function(aMatch, aMsg) {
needs more line breaks, I think

>+  let handled = true;
>+  // Loop over each raw CTCP message.
>+  for each (let message in ctcpMessages)
>+    handled &= ircHandlers.handleCTCPMessage(this, message);
try/catch?

>+var ctcpBase = {

>+    // Gets the local date and time from other clients.
>+    "TIME": function(aMessage) false,
if (aMessage.command == "PRIVMSG") {
  // CTCP TIME request
  LOG("Receiving CTCP TIME request from " + aMessage.nickname);
  this.sendCTCPMessage("TIME", [String(Date())], aMessage.nickname, true);
}
else {
  // CTCP TIME reply
  this._getConversation(aMessage.nickname)
      .writeMessage(aMessage.nickname,
                    _("ctcp.time.response", aMessage.ctcpParam, aMessage.nickname),
                    {system: true});
}
return true;

>diff --git a/chat/protocols/irc/ircHandlers.jsm b/chat/protocols/irc/ircHandlers.jsm

>+var ircHandlers = {

>+  /*
>+   * Object to hold the CTCP specifications, see above for the fields to
>+   * implement.
s/see above.*/expects the same fields as _ircHandlers/ perhaps?
>+   */
>+  _ctcpHandlers: [],

>+  _registerHandler: function(aArray, aHandler) {

>+    if (!aHandler.hasOwnProperty("commands")) {
consider using ("commands" in aHandler) in case it's on a prototype?
I guess you're guarding against /__proto__ and such silliness?

>+    aArray.push(aHandler);
>+    aArray.sort(function(a, b) b.priority - a.priority);
what happens if they have equal priority?

>+  _unregisterHandler: function(aArray, aHandler) {
>+    aArray.filter(function(h) h.name == aHandler.name)
>+          .forEach(function(h) aArray.splice(aArray.indexOf(h), 1));
is this clearer as
  aArray = aArray.filter(function(h) h.name != aHandler.name);
? That needs should be faster, at least... (O(n) instead of O(n^2), I think)

>+  _handleMessage: function(aHandlers, aAccount, aMessage, aCommand) {

>+      } catch (e) {
>+        // We want to catch an error here because one of our handlers are broken,
>+        // if we don't catch the error, the whole IRC plug-in will die.
>+        ERROR(JSON.stringify(aMessage));
Might want to say
 ERROR("Error running command " + aCommand + " with handler " + handler.name + ": " + JSON.stringify(aMessage));

Comment 34

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2011-12-16 01:01:01 UTC ***

(In reply to comment #23)
> >+    if (!aHandler.hasOwnProperty("commands")) {
> consider using ("commands" in aHandler) in case it's on a prototype?
> I guess you're guarding against /__proto__ and such silliness?
Not really, this isn't supplied by a user, but a developer so it's "trusted".

> >+    aArray.push(aHandler);
> >+    aArray.sort(function(a, b) b.priority - a.priority);
> what happens if they have equal priority?
That sucks. :( Don't do that. It's undefined behavior, do you have any suggestions? (Maybe arbitrarily just sort by name, in that case? Just to have a defined behavior.)

Comment 35

[Patrick Cloke [:clokep]]

Attachment: WIP 8

*** Original post on bio 507 as attmnt 1095 at 2011-12-29 21:29:00 UTC ***

Clear the ISON timer during unInit (to avoid leak).

Rewrote the ISON code to always send the most user names available.

WHOIS information (and tooltips) are working.

Mook's reviews (implements TIME, a few changes to the ircHandlers object).

Initial parsing of DCC messages (no implemented commands).

General clean up of code.

Initial support for ISUPPORT (allowing the server to tell the client what is supported): casemapping, maximum lengths, user mode prefixes, channel prefixes.

Fixed MODE command handling (that's set on the current user).

Starting to remove a prefixed underscore to some methods / properties which clearly aren't private.

Comment 36

[Patrick Cloke [:clokep]]

Comment on attachment 8352819 [details] [diff] [review]
WIP 7

*** Original change on bio 507 attmnt 1077 at 2011-12-29 21:29:45 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 37

[Patrick Cloke [:clokep]]

Attachment: Patch v1

*** Original post on bio 507 as attmnt 1115 at 2012-01-08 20:11:00 UTC ***

I think this is at the point it needs a "full" review from people.

Some more changes:
 - Left works properly.
 - Handle sending/receiving non-ASCII messages.
 - Uses SSL by default! (Current default settings are for freenode with SSL.)
 - Made some changes to the CTCP formatting (it was broken). Now includes two xpcshell tests: one to HTML and one to text.

This doesn't really depend on bug 954662 (bio 1230), but parts of this patch would be obsoleted/changed by it.

I could use help in sending/receiving non-ASCII messages, also the l10n strings could definitely use a once through.

Comment 38

[Patrick Cloke [:clokep]]

Comment on attachment 8352837 [details] [diff] [review]
WIP 8

*** Original change on bio 507 attmnt 1095 at 2012-01-08 20:11:22 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 39

[Patrick Cloke [:clokep]]

Attachment: Patch v2

*** Original post on bio 507 as attmnt 1145 at 2012-01-31 02:33:00 UTC ***

Changes include:
 - Supports mIRC's ridiculous coloring specification.
 - Cleaned up the commands file.
 - No longer checks if a nickname is valid before sending it to the server, we just let the server tell us if it's not.
 - Simplified connection error messages.
 - defaultAway was unused: was already using messenger.status.defaultIdleAwayMessage.
 - Cleaned up other strings per flo's suggestions.
 - Made the part command simpler (can only part the current room).
 - Removed the whois command, the output is only handled for tooltip situations. (whowas still exists, but I don't think the output is handled.)
 - Added irc.js and irc.manifest to package-manifest.in.
 - Fixed contract ID for http://hg.instantbird.org/instantbird/rev/1c6fbc25f7fc

Comment 40

[Patrick Cloke [:clokep]]

Comment on attachment 8352858 [details] [diff] [review]
Patch v1

*** Original change on bio 507 attmnt 1115 at 2012-01-31 02:33:15 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 41

[Patrick Cloke [:clokep]]

Attachment: Patch v2.1

*** Original post on bio 507 as attmnt 1147 at 2012-02-01 11:53:00 UTC ***

This fixes some issues with the patch:
 - Excludes my other unrelated changes.
 - Moved the icons instead of removing + adding them.
 - Fixed the naming of ircCTCP.jsm
 - Added the missing test file.
 - Added the passwordOptional flag.
 - A few instances of purple --> prpl.
 - Partially fixed MODE message.

Comment 42

[Patrick Cloke [:clokep]]

Comment on attachment 8352890 [details] [diff] [review]
Patch v2

*** Original change on bio 507 attmnt 1145 at 2012-02-01 11:53:20 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 43

[Florian Quèze [:florian]]

Attachment: Interdiff review (mostly irc.js)

*** Original post on bio 507 as attmnt 1148 at 2012-02-01 22:59:00 UTC ***

I'm trying a new form of review that I think may let us get things done faster: I applied the patch and directly edited the files, fixing obvious brokenness when encountered, fixing nits rather than describing them, and adding XXXFlo comments where I would usually have written a review comment. I'm attaching an interdiff.

For now, irc.js is the only file that I've read from begin to end.

Some more general comments:
- The messages displayed in the "server tab" with the "notification" flag but not "system" look broken.
- do we need that server tab at all? Or maybe there should be an advanced account setting to enable it? (or an about config pref to globally enable it for all IRC accounts at once)

Comment 44

[Patrick Cloke [:clokep]]

Attachment: Patch v2.2

*** Original post on bio 507 as attmnt 1149 at 2012-02-02 11:21:00 UTC ***

This patch takes into account flo's interdiff (attachment 8352893 [details] [diff] [review] (bio-attmnt 1148)) and fixes some of the XXXFlo comments.

Comment 45

[Patrick Cloke [:clokep]]

Comment on attachment 8352892 [details] [diff] [review]
Patch v2.1

*** Original change on bio 507 attmnt 1147 at 2012-02-02 11:22:28 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 46

[Patrick Cloke [:clokep]]

Comment on attachment 8352893 [details] [diff] [review]
Interdiff review (mostly irc.js)

*** Original change on bio 507 attmnt 1148 at 2012-02-02 11:22:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 47

[Florian Quèze [:florian]]

Attachment: Interdiff review

*** Original post on bio 507 as attmnt 1150 at 2012-02-02 22:20:00 UTC ***

I've now looked at all the files.

Some overall comments:
- we really need to find an acceptable solution for the encoding issue. I'm wondering if just adding a try/catch around the converter call and falling back to the unconverted string when the conversion throws would be acceptable.
- your length computations don't take into account non-ascii characters.
- we need to check the behavior when disconnecting/reconnecting (in which state are the conversations after that?), and the interactions with the offline status.
- please free us of the server tabs! Add a either a "system" or an "incoming" flag to these messages displayed with only the "notification" flag, and default to no server tab (either with an account specific pref, or an about:config pref).

This looks pretty good otherwise, and I'll r+ once all my comments are addressed (addressed here doesn't necessarily mean a code change, it can also just be an answer :-)).

Comment 48

[Patrick Cloke [:clokep]]

Attachment: Patch v2.3

*** Original post on bio 507 as attmnt 1166 at 2012-02-10 00:40:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 49

[Patrick Cloke [:clokep]]

Comment on attachment 8352894 [details] [diff] [review]
Patch v2.2

*** Original change on bio 507 attmnt 1149 at 2012-02-10 00:40:04 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 50

[Patrick Cloke [:clokep]]

Comment on attachment 8352895 [details] [diff] [review]
Interdiff review

*** Original change on bio 507 attmnt 1150 at 2012-02-10 00:40:04 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 51

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2012-02-13 02:01:48 UTC ***

(In reply to comment #31)
> Some overall comments:
> - we really need to find an acceptable solution for the encoding issue. I'm
> wondering if just adding a try/catch around the converter call and falling back
> to the unconverted string when the conversion throws would be acceptable.
I agree. Do we have any ideas for this?

> - your length computations don't take into account non-ascii characters.
I'm still unsure of whether the specification refers to bytes or to characters. It says:
> these messages SHALL NOT exceed 512 characters in length, counting all
> characters including the trailing CR-LF

But it also refers to:
>   No specific character set is specified. The protocol is based on a
>   set of codes which are composed of eight (8) bits, making up an
>   octet.  Each message may be composed of any number of these octets;
>   however, some octet values are used for control codes, which act as
>   message delimiters.
>
>   Regardless of being an 8-bit protocol, the delimiters and keywords
>   are such that protocol is mostly usable from US-ASCII terminal and a
>   telnet connection.

This (to me) implies that the RFC refers to a "character" as an 8-bit entity. Do we have any opinions about this? I was thinking of asking in ChatZilla if they have thoughts.

> - we need to check the behavior when disconnecting/reconnecting (in which state
> are the conversations after that?), and the interactions with the offline
> status.
I agree! There's been some changes to the way this was handled globally too, hopefully I account for those after fixing bug 954631 (bio 1199).

> - please free us of the server tabs! Add a either a "system" or an "incoming"
> flag to these messages displayed with only the "notification" flag, and default
> to no server tab (either with an account specific pref, or an about:config
> pref).
The server tab is now optional. Where do I use the notification flag without any other flags? The only place I see it used is with the incoming flag.

Comment 52

[Patrick Cloke [:clokep]]

Attachment: Patch v3

*** Original post on bio 507 as attmnt 1176 at 2012-02-17 01:53:00 UTC ***

Changes:
 - Handles encoding with try-catch blocks.
 - Now counts bytes instead of characters.
 - No more server tab by default.

I still need to check the (dis)connecting logic. But I've never had issues with it in my testing. Is there a specific fear?

Comment 53

[Patrick Cloke [:clokep]]

Comment on attachment 8352911 [details] [diff] [review]
Patch v2.3

*** Original change on bio 507 attmnt 1166 at 2012-02-17 01:53:23 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 54

[Patrick Cloke [:clokep]]

Attachment: Patch v4

*** Original post on bio 507 as attmnt 1181 at 2012-02-21 01:53:00 UTC ***

Fully redone (dis)connect(ion|ed|ing) logic. Hopefully this is easier to understand! Seems that bug 954631 (bio 1199) handled a lot of this for us. :)

Looking over my to-do list, the last thing I think needs to be done before landing is handling of proxies. This could probably even be done as a follow up. I don't want to dig my head into the proxy code right now though. :(

Comment 55

[Patrick Cloke [:clokep]]

Comment on attachment 8352924 [details] [diff] [review]
Patch v3

*** Original change on bio 507 attmnt 1176 at 2012-02-21 01:53:30 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 56

[Florian Quèze [:florian]]

*** Original post on bio 507 at 2012-02-21 17:59:34 UTC ***

Comment on attachment 8352930 [details] [diff] [review] (bio-attmnt 1181)
Patch v4

I looked at the interdiff between attachment 8352894 [details] [diff] [review] (bio-attmnt 1149)+1150 and this, here are my comments:

18:14:12 - flo: "The topic for %1$S was removed." $S is enough if there's only one parameter
18:14:36 - flo: especially as the comment above it is "%S is the conversation name."
18:20:37 - flo: "// Convert the byte offsets and lengths into character indexes." should be removed, it's unrelated to the code you reused :)
18:23:41 - flo: |setTimeout(this.gotDisconnected.bind(this), 5 * 1000);| looks dangerous
18:25:11 - flo: clokep_work: I'm assuming it doesn't take 5s to disconnect, but we will mark the account as disconnected only after the server has replied. What happens if in less than 5 seconds the server replies, we mark the account as disconnected, and the user reconnects the account?
18:31:13 - flo:  // If the user left, mark the conversation as no longer being active. conversation.left = true; (in function leftRoom). Isn't there a notification that needs to be sent to the conversation in that case?
18:32:15 - flo: http://lxr.instantbird.org/instantbird/search?string=update-conv-chatleft
18:34:57 - flo: maybe this could be done in jsProtoHelper in a left setter?
18:38:38 - flo: you removed the code using "message.topicSetter", but it's still in the properties file
18:39:57 - flo: this._motd = [aMessage.params[1].slice(2,-2)];
18:40:02 - flo: a space is missing after the comma
18:43:44 - flo: nit: "// Use the brandShortName as the client version." I would remove the first "the" from that sentence, as brandShortName here is a variable name, not "branch short name" :)

Comment 57

[Patrick Cloke [:clokep]]

Attachment: Patch v5

*** Original post on bio 507 as attmnt 1189 at 2012-02-22 01:21:00 UTC ***

 - Fixed properties file.
 - Fixed byte counting comment.
 - Kill the QUIT timer if a response is received (and on uninit).
 - Leaving and rejoining a chat updates the icon correctly.
 - Fixes sending private messages.

Comment 58

[Patrick Cloke [:clokep]]

Comment on attachment 8352930 [details] [diff] [review]
Patch v4

*** Original change on bio 507 attmnt 1181 at 2012-02-22 01:21:35 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 59

[Florian Quèze [:florian]]

Attachment: Interdiff review (patch v5)

*** Original post on bio 507 as attmnt 1190 at 2012-02-22 16:34:00 UTC ***

Thanks for fixing quickly all the issues I spotted yesterday.

I've continued to test this and found more issues. I've fixed most of them already (see attached interdiff).

Things left to do before landing:
- Fix the TOPIC command and the topic setter
- figure out what goes wrong in some gotDisconnected calls when disconnecting an account from the account manager
- Find why there are shutdown leaks if an IRC account was connected... :-(

- I haven't tried putting an account with a password, I'll need to test that soon.

In a follow-up, I think we will want to improve the tooltips.

There are issues with the offline state, but JS-XMPP shares them, so I guess I should just go fix socket.jsm, and not hold JS-IRC because of socket.jsm's brokenness.


Here are the notes that I took while testing/debugging this:

- encoding
(FIXED, it was almost right, the only issue was that the converted wasn't created when the desired encoding is UTF-8, although the string we receive from the socket is in the local encoding, so we still need a convertion to handle an UTF-8 encoded string received over the network)
- proxy
FIXED, the proxy service just seems to do a poor job with non-http schemes
- Can't set a channel topic:
-- the topic command is broken (doesn't send the channel name to the server)
-- the chat conversations don't have a topic setter (Warning: setting a property that has only a getter
Source File: file:///.../components/imConversations.js
Line: 333)
- sending a message containing a \n
(the server replies :concrete.mozilla.org 421 testib For :Unknown command)
(FIXED, just needed   get noNewlines() true,)
- Error: str is undefined
Source File: file:///.../components/irc.js
Line: 413
(when receiving :concrete.mozilla.org NOTICE AUTH :*** Looking up your hostname...)
(FIXED, "NOTICE" was calling privmsg instead of serverMessage for server notice messages...)
- re-joining a channel whose nicklist contained nicks with upper case letters caused lots of JS errors related to the nicklist.
(FIXED, the removeAllParticipants function was sending the chat-buddy-remove signal with the normalized nicks instead of the raw nicks)
- disconnecting an account and then reconnecting it caused nicklist errors because the user's nick was removed without firing the conversation notification
(FIXED)
- various JS errors that prevented deleting an account
Before I started hacking this, deleting a JS-IRC account from the account manager made it disappear from the account manager and *connected* the account!
FIXED
- JS error when using the /time command:
Error: uncaught exception: [Exception... "'[JavaScript Error: "aParams is undefined" {file: "resource:///modules/ircCommands.jsm" line: 112}]' when calling method: [imICommand::run]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///.../components/imCommands.js :: <TOP_LEVEL> :: line 245"  data: yes]
(FIXED, simpleCommand was broken for commands without parameter)
- broken VERSION CTCP handler (Error: l10nHelper is not defined
Source File: resource:///modules/ircCTCP.jsm
Line: 261)
(FIXED, added Cu.import("resource:///modules/imXPCOMUtils.jsm");)
- (I don't understand this warning)
Warning: reference to undefined property c.usageContext
Source File: file:///.../components/imCommands.js
Line: 204
- Unhandled messages (can be fixed after landing):
:concrete.mozilla.org 307 testib flo :is a registered nick
:concrete.mozilla.org 671 testib flo :is using a Secure Connection
- The current parsing of the 317 reply seems incomplete, the Mozilla server for example sends:
:concrete.mozilla.org 317 testib flo 1569 1329906345 :seconds idle, signon time

Comment 60

[Patrick Cloke [:clokep]]

Comment on attachment 8352940 [details] [diff] [review]
Interdiff review (patch v5)

*** Original change on bio 507 attmnt 1190 at 2012-02-22 21:05:49 UTC ***

All these changes look good. :) Thanks for figuring out the proxy and encoding messes!

Comment 61

[Patrick Cloke [:clokep]]

Attachment: Patch v6

*** Original post on bio 507 as attmnt 1191 at 2012-02-23 20:43:00 UTC ***

 - Setting topic works.
 - Receiving topic works.
 - Handles response 433 (nick already in use).
 - Includes another diff from flo for handling unicode converters a little neater.

Comment 62

[Patrick Cloke [:clokep]]

Comment on attachment 8352939 [details] [diff] [review]
Patch v5

*** Original change on bio 507 attmnt 1189 at 2012-02-23 20:43:59 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 63

[Patrick Cloke [:clokep]]

Comment on attachment 8352940 [details] [diff] [review]
Interdiff review (patch v5)

*** Original change on bio 507 attmnt 1190 at 2012-02-23 20:43:59 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 64

[Florian Quèze [:florian]]

Attachment: Interdiff review (patch v6)

*** Original post on bio 507 as attmnt 1192 at 2012-02-23 23:35:00 UTC ***

Some more changes:
- Somehow fixes the JS error when disconnecting during shutdown. I'm not super happy with this as the code still seems a bit fragile, but at least it seems in working conditions now.
- Fixed the memory leak at shutdown. The cause was the missing GenericConv{Chat,IM}Prototype.unInit calls.
- It seems this._isOnTimer was never canceled when disconnecting an account. I'm clearing it in gotDisconnected now. Please tell me if I missed something here :)
- topics really work.

Comment 65

[Florian Quèze [:florian]]

Attachment: Interdiff review 2 (patch v6)

*** Original post on bio 507 as attmnt 1194 at 2012-02-24 12:15:00 UTC ***

Some more changes:
- fix HTML special characters handling
- fix sending the password
- instantbird.com (instead of .org) in the default quit message.

Comment 66

[Patrick Cloke [:clokep]]

Comment on attachment 8352944 [details] [diff] [review]
Interdiff review 2 (patch v6)

*** Original change on bio 507 attmnt 1194 at 2012-02-24 13:08:25 UTC ***

These changes look fine. I do wonder if converting entities should be done by the topic setting UI code instead of the protocol.

Comment 67

[Patrick Cloke [:clokep]]

Comment on attachment 8352942 [details] [diff] [review]
Interdiff review (patch v6)

*** Original change on bio 507 attmnt 1192 at 2012-02-24 13:20:00 UTC ***

These changes look good, as discussed on IRC I want the command to also set the topic in the same way as the UI (I only like to send messages, i.e. actually implement the protocol, in one place for each command).

Comment 68

[Florian Quèze [:florian]]

Attachment: Some more changes

*** Original post on bio 507 as attmnt 1195 at 2012-02-24 15:28:00 UTC ***

- the topic command now uses the topic setter, as requested in comment 44.
- no more error when disconnecting an account from the account manager.

Comment 69

[Patrick Cloke [:clokep]]

Comment on attachment 8352946 [details] [diff] [review]
Some more changes

*** Original change on bio 507 attmnt 1195 at 2012-02-24 20:33:52 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 70

[Patrick Cloke [:clokep]]

Attachment: Patch v7

*** Original post on bio 507 as attmnt 1196 at 2012-02-24 20:54:00 UTC ***

Includes the last few sets of changes and also does NOT log sending the password (as flo asked for online). This is done by allowing you pass in false to the sendData/sendString commands for a socket to kill the logging.

Comment 71

[Patrick Cloke [:clokep]]

Comment on attachment 8352941 [details] [diff] [review]
Patch v6

*** Original change on bio 507 attmnt 1191 at 2012-02-24 20:54:25 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 72

[Patrick Cloke [:clokep]]

Comment on attachment 8352942 [details] [diff] [review]
Interdiff review (patch v6)

*** Original change on bio 507 attmnt 1192 at 2012-02-24 20:54:25 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 73

[Patrick Cloke [:clokep]]

Comment on attachment 8352944 [details] [diff] [review]
Interdiff review 2 (patch v6)

*** Original change on bio 507 attmnt 1194 at 2012-02-24 20:54:25 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 74

[Patrick Cloke [:clokep]]

Comment on attachment 8352946 [details] [diff] [review]
Some more changes

*** Original change on bio 507 attmnt 1195 at 2012-02-24 20:54:25 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 75

[Patrick Cloke [:clokep]]

Attachment: Patch v7.1

*** Original post on bio 507 as attmnt 1202 at 2012-02-26 18:33:00 UTC ***

This updates the socket code to optionally take a separate string to log the data. Additionally it includes the changes to conversation.xml which was missed in the last patch.

Comment 76

[Patrick Cloke [:clokep]]

Comment on attachment 8352947 [details] [diff] [review]
Patch v7

*** Original change on bio 507 attmnt 1196 at 2012-02-26 18:33:34 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 77

[Patrick Cloke [:clokep]]

Attachment: Patch v7.2

*** Original post on bio 507 as attmnt 1203 at 2012-02-26 18:45:00 UTC ***

Fix an issue with spacing and avoid including an unrelated patch.

Comment 78

[Patrick Cloke [:clokep]]

Comment on attachment 8352953 [details] [diff] [review]
Patch v7.1

*** Original change on bio 507 attmnt 1202 at 2012-02-26 18:45:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 79

[Patrick Cloke [:clokep]]

Attachment: Patch v8

*** Original post on bio 507 as attmnt 1204 at 2012-02-27 02:20:00 UTC ***

Defaults to non-SSL for freenode.

Fixes the log directories.

Comment 80

[Patrick Cloke [:clokep]]

Comment on attachment 8352954 [details] [diff] [review]
Patch v7.2

*** Original change on bio 507 attmnt 1203 at 2012-02-27 02:20:54 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 81

[Florian Quèze [:florian]]

Comment on attachment 8352955 [details] [diff] [review]
Patch v8

*** Original change on bio 507 attmnt 1204 at 2012-02-27 14:22:02 UTC ***

Time to land... \o/

Comment 82

[Florian Quèze [:florian]]

Attachment: Additional changes before landing

*** Original post on bio 507 as attmnt 1205 at 2012-02-27 14:23:00 UTC ***

We discussed these final changes before landing today on IRC.

Comment 83

[Florian Quèze [:florian]]

*** Original post on bio 507 at 2012-02-27 14:28:56 UTC ***

https://hg.instantbird.org/instantbird/rev/920499b108a1 (attachment 8352955 [details] [diff] [review] (bio-attmnt 1204) + 1205)

Resolving to FIXED!

We already know a few issues that should be addressed in follow-ups. (Freenode SSL reconnection issues; source displayed in "entered the room" system messages; some tooltip improvements, ...)

Patrick, thanks for pushing this forward over the course of the last 2 years!

Comment 84

[Florian Quèze [:florian]]

*** Original post on bio 507 at 2012-02-27 14:30:11 UTC ***

(actually setting the resolution to fixed is better than just talking about it)

Comment 85

[Patrick Cloke [:clokep]]

*** Original post on bio 507 at 2012-02-27 15:41:45 UTC ***

Moving to the new IRC component.