955328 - Use Javascript default parameters where applicable

Status: RESOLVED FIXED

(Chat Core :: General, enhancement)

Description

[Benedikt Pfeifer]

*** Original post on bio 1893 at 2013-03-10 22:16:00 UTC ***

With Mozilla 15, we got support for Javascript default parameters [1]. We should find out where it would make sense to use them and update the code accordingly.

These commands might help finding places where they might be applicable (limiting the search to js/jsm files for now):

"if (!aParam) aParam = ...;" - pattern:
  hg grep -Ire:.*\.jsm?$ -n "if\s+\(\s*!\s*a[A-Z]\w*\)"

"= aParam || ..." - pattern:
  hg grep -Ire:.*\.jsm?$ -n "\s+=\s+a[A-Z]\w*\s+\|\|"

[1] https://developer.mozilla.org/en-US/docs/JavaScript/Reference/default_parameters

Comment 1

[Florian Quèze [:florian]]

*** Original post on bio 1893 at 2013-03-10 22:20:54 UTC ***

(In reply to comment #0)

> "= aParam || ..." - pattern:
>   hg grep -Ire:.*\.jsm?$ -n "\s+=\s+a[A-Z]\w*\s+\|\|"

You may even want to search without "= " in case the parameter is used only once as the parameter of another function.

Comment 2

[Patrick Cloke [:clokep]]

Attachment: Patch v1

*** Original post on bio 1893 as attmnt 2265 at 2013-03-11 10:51:00 UTC ***

This might not catch every case, Florian suggested searching for !! as well.

This was only briefly tested, but I wanted to get it up for review in case someone has time to look at it.

Comment 3

[Patrick Cloke [:clokep]]

Attachment: Patch v2

*** Original post on bio 1893 as attmnt 2266 at 2013-03-11 23:51:00 UTC ***

Fixes the XMPP default params.

Comment 4

[Patrick Cloke [:clokep]]

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

*** Original change on bio 1893 attmnt 2265 at 2013-03-11 23:51:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[Benedikt Pfeifer]

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

*** Original change on bio 1893 attmnt 2266 at 2013-03-13 00:43:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Benedikt Pfeifer]

*** Original post on bio 1893 at 2013-03-13 00:43:46 UTC ***

Comment on attachment 8354031 [details] [diff] [review] (bio-attmnt 2266)
Patch v2

I haven't actually tried the changes yet but I wanted to finish the comments I had tonight.

>diff --git a/chat/modules/imXPCOMUtils.jsm b/chat/modules/imXPCOMUtils.jsm
>@@ -84,24 +84,23 @@ function scriptError(aModule, aLevel, aM
>   if ("imAccount" in this)
>     this.imAccount.logDebugMessage(scriptError, aLevel);
> }
>-function initLogModule(aModule, aThis)
>+function initLogModule(aModule, aThis = {})
> {
>-  let obj = aThis || {};
>-  obj.DEBUG = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_DEBUG);
>-  obj.LOG   = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_LOG);
>-  obj.WARN  = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_WARNING);
>-  obj.ERROR = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_ERROR);
>-  return obj;
>+  aThis.DEBUG = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_DEBUG);
>+  aThis.LOG   = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_LOG);
>+  aThis.WARN  = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_WARNING);
>+  aThis.ERROR = scriptError.bind(obj, aModule, Ci.imIDebugMessage.LEVEL_ERROR);
>+  return aThis;

You'll need to replace "obj" with "aThis" in the bind-arguments, too.

>@@ -165,32 +164,32 @@ function executeSoon(aFunction)
> 
> // Similar to Object.hasOwnProperty, but doesn't fail if the object
> // has a hasOwnProperty property set.
> function hasOwnProperty(aObject, aPropertyName)
>   Object.prototype.hasOwnProperty.call(aObject, aPropertyName)
> 
> /* Common nsIClassInfo and QueryInterface implementation
>  * shared by all generic objects implemented in this file. */
>-function ClassInfo(aInterfaces, aDescription)
>+function ClassInfo(aInterfaces, aDescription = "JS Proto Object")
> {
>   if (!(this instanceof ClassInfo))
>     return new ClassInfo(aInterfaces, aDescription);

I'm not sure what the "this instanceof"-check is for but even though it slightly
changes the flow here (before, the constructor might have been called with 
aDescription = undefined and would have set the default value itself) it should
be OK to have this change.

>diff --git a/chat/modules/socket.jsm b/chat/modules/socket.jsm
>@@ -132,26 +132,26 @@ const Socket = {
>   readWriteTimeout: 0,
> 
>   /*
>    *****************************************************************************
>    ******************************* Public methods ******************************
>    *****************************************************************************
>    */
>   // Synchronously open a connection.
>-  connect: function(aHost, aPort, aSecurity, aProxy) {
>+  connect: function(aHost, aPort, aSecurity = [], aProxy = undefined) {

I'm surprised that you can set the default to "undefined" but it should be OK
since this is the value the parameter would have if it weren't specified in a call.

>@@ -200,50 +200,50 @@ const Socket = {
> 
>   // Listen for a connection on a port.
>   // XXX take a timeout and then call stopListening
>-  listen: function(port) {
>-    this.LOG("Listening on port " + port);
>+  listen: function(aPort) {
>+    this.LOG("Listening on port " + aPort);
> 
>-    this.serverSocket = new ServerSocket(port, false, -1);
>+    this.serverSocket = new ServerSocket(aPort, false, -1);

Unrelated change but since you're editing the file anyways, it's fine with me.

>-  sendData: function(/* string */ aData, aLoggedData) {
>-    this.LOG("Sending:\n" + (aLoggedData || aData));
>+  sendData: function(/* string */ aData, aLoggedData = aData) {
>+    this.LOG("Sending:\n" + aLoggedData);

Referencing an earlier parameter works indeed, I've tried that.

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
>@@ -810,17 +809,18 @@ ircAccount.prototype = {
>     }
>     else if (type == Ci.imIStatusInfo.STATUS_AVAILABLE && this.isAway)
>       this.sendMessage("AWAY"); // Mark as back.
>   },
> 
>   // The user's user mode.
>   _modes: [],
>   _userModeReceived: false,
>-  setUserMode: function(aNick, aNewModes, aSetter, aDisplayFullMode) {
>+  setUserMode: function(aNick, aNewModes, aSetter = this._currentServerName,
>+                        aDisplayFullMode = false) {

aDisplayFullMode has only a default value because it's preceeded by a parameter
with a default value and needs to have one. You used "undefined" in the (imo)
similar case with "aProxy" in socket.jsm's connect() method. Do we want to be
consistent here? Ignore this comment if you think that it is bikeshedding.

>@@ -1164,20 +1162,19 @@ ircAccount.prototype = {
>     // If no more CAP messages are being handled, notify the server.
>     if (!this._caps.length)
>       this.sendMessage("CAP", "END");
>   },
> 
>   // Used to wait for a response from the server.
>   _quitTimer: null,
>   // RFC 2812 Section 3.1.7.
>-  quit: function(aMessage) {
>+  quit: function(aMessage = this.getString("quitmsg") || undefined) {
>     this._reportDisconnecting(Ci.prplIAccount.NO_ERROR);
>-    this.sendMessage("QUIT",
>-                     aMessage || this.getString("quitmsg") || undefined);
>+    this.sendMessage("QUIT", aMessage);

In my opinion the previous code was a bit clearer of how it is falling through the
different cases:
No aMessage? -> Try string from options. No string from options? -> Use undefined.
Now we would be using two different concepts (default parameters and this "||"-fallback) instead.

>     let message = aCommand;
>-    // If aParams is empty, then use an empty array. If aParams is not an array,
>-    // consider it to be a single parameter and put it into an array.
>-    let params = !aParams ? [] : Array.isArray(aParams) ? aParams : [aParams];
>+    // If aParams is not an array, consider it to be a single parameter and put
>+    // it into an array.
>+    let params = Array.isArray(aParams) ? aParams : [aParams];

Thanks! It's much more readable now :)

>@@ -1339,22 +1336,23 @@ ircAccount.prototype = {
>         this.ERROR("Socket error:", e);
>         this.gotDisconnected(Ci.prplIAccount.ERROR_NETWORK_ERROR,
>                              _("connection.error.lost"));
>       }
>     }
>   },
> 
>   // CTCP messages are \001<COMMAND> [<parameters>]*\001.
>-  sendCTCPMessage: function(aCommand, aParams, aTarget, aIsNotice) {
>+  sendCTCPMessage: function(aCommand, aParams = [], aTarget = undefined,
>+                            aIsNotice = false) {

Same question about using undefined or a particular value (false) for "necessary"
default values (aParams is the interesting parameter, the others just got the
bad luck to come later).

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>@@ -945,20 +945,19 @@ const XMPPAccountPrototype = {
>       else if (state == "paused")
>         this._conv[norm].updateTyping(Ci.prplIConvIM.TYPED);
>     }
>     else
>       this._conv[norm].supportChatStateNotifications = false;
>   },
> 
>   /* Called when there is an error in the xmpp session */
>-  onError: function(aError, aException) {
>-    if (aError === null || aError === undefined)
>-      aError = Ci.prplIAccount.ERROR_OTHER_ERROR;
>-    this._disconnect(aError, aException.toString());
>+  onError: function(aError = Ci.prplIAccount.ERROR_OTHER_ERROR,
>+                    aException = "") {
>+    this._disconnect(aError, aException);

Unfortunately you can't remove the null-check (which makes default parameters
useless here). Null is not undefined and therefore the default parameter
wouldn't be used in this case. (I've tried!)

>diff --git a/instantbird/content/nsContextMenu.js b/instantbird/content/nsContextMenu.js

The changes in this file look fine but isn't it one that we copied from Firefox?
Aren't we trying to have as little changes in these files as possible?

Comment 7

[Patrick Cloke [:clokep]]

*** Original post on bio 1893 at 2013-03-13 01:26:11 UTC ***

(In reply to comment #4)
> Comment on attachment 8354031 [details] [diff] [review] (bio-attmnt 2266) [details]
> Patch v2
> 

> >@@ -165,32 +164,32 @@ function executeSoon(aFunction)
> > /* Common nsIClassInfo and QueryInterface implementation
> >  * shared by all generic objects implemented in this file. */
> >-function ClassInfo(aInterfaces, aDescription)
> >+function ClassInfo(aInterfaces, aDescription = "JS Proto Object")
> > {
> >   if (!(this instanceof ClassInfo))
> >     return new ClassInfo(aInterfaces, aDescription);
> 
> I'm not sure what the "this instanceof"-check is for but even though it
> slightly
> changes the flow here (before, the constructor might have been called with 
> aDescription = undefined and would have set the default value itself) it should
> be OK to have this change.
I'm not positive either. Florian should know. (That code seems to be recursive, which is confusing...) It might actually be OK to just throw if aInterfaces or aDescription is undefined?

> >   // Synchronously open a connection.
> >-  connect: function(aHost, aPort, aSecurity, aProxy) {
> >+  connect: function(aHost, aPort, aSecurity = [], aProxy = undefined) {
> 
> I'm surprised that you can set the default to "undefined" but it should be OK
> since this is the value the parameter would have if it weren't specified in a
> call.
Why? undefined is a legitimate JS value. It works fine, I've tested it.

> >-  sendData: function(/* string */ aData, aLoggedData) {
> >-    this.LOG("Sending:\n" + (aLoggedData || aData));
> >+  sendData: function(/* string */ aData, aLoggedData = aData) {
> >+    this.LOG("Sending:\n" + aLoggedData);
> 
> Referencing an earlier parameter works indeed, I've tried that.
[1] shows that this is explicitly supported:
> The default values should be computed in the scope of the callee, thus
> enabling use of
>   any parameters to the left of the one being given its default value, and of
>   any lexical and free (global) variables in enclosing functions and the top
>   level

> >diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
> >@@ -810,17 +809,18 @@ ircAccount.prototype = {
> >-  setUserMode: function(aNick, aNewModes, aSetter, aDisplayFullMode) {
> >+  setUserMode: function(aNick, aNewModes, aSetter = this._currentServerName,
> >+                        aDisplayFullMode = false) {
> 
> aDisplayFullMode has only a default value because it's preceeded by a parameter
> with a default value and needs to have one. You used "undefined" in the (imo)
> similar case with "aProxy" in socket.jsm's connect() method. Do we want to be
> consistent here? Ignore this comment if you think that it is bikeshedding.
aDisplayFullMode is a boolean, if it isn't supplied it's "false" by default, I think it's much clearer to just set the default to false instead of undefined.

> >@@ -1164,20 +1162,19 @@ ircAccount.prototype = {
> >-  quit: function(aMessage) {
> >+  quit: function(aMessage = this.getString("quitmsg") || undefined) {
> >     this._reportDisconnecting(Ci.prplIAccount.NO_ERROR);
> >-    this.sendMessage("QUIT",
> >-                     aMessage || this.getString("quitmsg") || undefined);
> >+    this.sendMessage("QUIT", aMessage);
> 
> In my opinion the previous code was a bit clearer of how it is falling through
> the
> different cases:
> No aMessage? -> Try string from options. No string from options? -> Use
> undefined.
> Now we would be using two different concepts (default parameters and this
> "||"-fallback) instead.
I agree this is kind of confusing, would it make sense to keep the default (this.getString("quitmsg")) and move "|| undefined" back where it was, or just put the whole thing back how it was?

> >@@ -1339,22 +1336,23 @@ ircAccount.prototype = {
> >   // CTCP messages are \001<COMMAND> [<parameters>]*\001.
> >-  sendCTCPMessage: function(aCommand, aParams, aTarget, aIsNotice) {
> >+  sendCTCPMessage: function(aCommand, aParams = [], aTarget = undefined,
> >+                            aIsNotice = false) {
> 
> Same question about using undefined or a particular value (false) for
> "necessary"
> default values (aParams is the interesting parameter, the others just got the
> bad luck to come later).
I again attest that it makes sense for aIsNotice to default to false since it's a Boolean. aTarget is a string, so it makes sense to default it to undefined.

> >diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
> >@@ -945,20 +945,19 @@ const XMPPAccountPrototype = {
> >   /* Called when there is an error in the xmpp session */
> >-  onError: function(aError, aException) {
> >-    if (aError === null || aError === undefined)
> >-      aError = Ci.prplIAccount.ERROR_OTHER_ERROR;
> >-    this._disconnect(aError, aException.toString());
> >+  onError: function(aError = Ci.prplIAccount.ERROR_OTHER_ERROR,
> >+                    aException = "") {
> >+    this._disconnect(aError, aException);
> 
> Unfortunately you can't remove the null-check (which makes default parameters
> useless here). Null is not undefined and therefore the default parameter
> wouldn't be used in this case. (I've tried!)
But did you check if we ever actually send null? I couldn't find any place that we do. I also think this is an abuse of an API and we should send undefined (or nothing at all). I actually greatly dislike the usage of onError throughout the XMPP code...

> >diff --git a/instantbird/content/nsContextMenu.js b/instantbird/content/nsContextMenu.js
> 
> The changes in this file look fine but isn't it one that we copied from
> Firefox?
> Aren't we trying to have as little changes in these files as possible?
I don't know what our policy is on these files. Florian?

Thanks for the review. :)

[1] http://wiki.ecmascript.org/doku.php?id=harmony:parameter_default_values#goals

Comment 8

[Florian Quèze [:florian]]

*** Original post on bio 1893 at 2013-03-13 09:27:34 UTC ***

(In reply to comment #4)

> > /* Common nsIClassInfo and QueryInterface implementation
> >  * shared by all generic objects implemented in this file. */
> >-function ClassInfo(aInterfaces, aDescription)
> >+function ClassInfo(aInterfaces, aDescription = "JS Proto Object")
> > {
> >   if (!(this instanceof ClassInfo))
> >     return new ClassInfo(aInterfaces, aDescription);
> 
> I'm not sure what the "this instanceof"-check is for

It's a hack to make the "new " keyword optional.

If you have this code:
function foo() { /* set this.bar */ }
foo.prototype = {
 a: ...
};

To create an instance of foo, you need to call |new foo()|. If you forget the 'new' keyword, just calling foo() will execute code in the constructor, but won't return a new instance of foo.


I think false is better than undefined for the default value of boolean parameters (but I think clokep already said that in a previous reply).

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v3

*** Original post on bio 1893 as attmnt 2277 at 2013-03-14 22:32:00 UTC ***

Meets the review comments.

Comment 10

[Patrick Cloke [:clokep]]

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

*** Original change on bio 1893 attmnt 2266 at 2013-03-14 22:32:30 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[Benedikt Pfeifer]

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

*** Original change on bio 1893 attmnt 2277 at 2013-04-17 19:31:36 UTC ***

> > >@@ -1164,20 +1162,19 @@ ircAccount.prototype = {
> > >-  quit: function(aMessage) {
> > >+  quit: function(aMessage = this.getString("quitmsg") || undefined) {
> > >     this._reportDisconnecting(Ci.prplIAccount.NO_ERROR);
> > >-    this.sendMessage("QUIT",
> > >-                     aMessage || this.getString("quitmsg") || undefined);
> > >+    this.sendMessage("QUIT", aMessage);
> > 
> > In my opinion the previous code was a bit clearer of how it is falling through
> > the
> > different cases:
> > No aMessage? -> Try string from options. No string from options? -> Use
> > undefined.
> > Now we would be using two different concepts (default parameters and this
> > "||"-fallback) instead.
> I agree this is kind of confusing, would it make sense to keep the default
> (this.getString("quitmsg")) and move "|| undefined" back where it was, or just
> put the whole thing back how it was?

I'm fine with moving it back.
Thanks for the update and sorry for the delay, I mistakenly assumed this review would take a lot more time than it actually did.

Comment 12

[Florian Quèze [:florian]]

*** Original post on bio 1893 at 2013-04-17 22:41:41 UTC ***

Comment on attachment 8354042 [details] [diff] [review] (bio-attmnt 2277)
Patch v3

>-function getTheme(aName)
>+function getTheme(aName = Services.prefs.getCharPref(kEmoticonsThemePref))
> {
>-  let name = aName || Services.prefs.getCharPref(kEmoticonsThemePref);
>+  let name = aName;

Why do we still need the |name| variable?

Also, is the error reporting correct if Services.prefs.getCharPref(kEmoticonsThemePref) throws?

(I'm generally not thrilled by using function calls as default parameter values, but if it doesn't hurt debugging, I don't have any serious objection.)


>   // CTCP messages are \001<COMMAND> [<parameters>]*\001.
>-  sendCTCPMessage: function(aCommand, aParams, aTarget, aIsNotice) {
>+  sendCTCPMessage: function(aCommand, aParams = [], aTarget = undefined,
>+                            aIsNotice = false) {
>     // Combine the CTCP command and parameters into the single IRC param.
>     let ircParam = aCommand;
>-    // If aParams is empty, then use an empty array. If aParams is not an array,
>-    // consider it to be a single parameter and put it into an array.
>-    let params = !aParams ? [] : Array.isArray(aParams) ? aParams : [aParams];
>+    // If aParams is not an array, consider it to be a single parameter and put
>+    // it into an array.
>+    let params = Array.isArray(aParams) ? aParams : [aParams];
>     if (params.length)
>       ircParam += " " + params.join(" ");

Doesn't this change the behavior if aParams is null or ""? Are we sure this is OK?


>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>   /* Called when there is an error in the xmpp session */
>-  onError: function(aError, aException) {
>-    if (aError === null || aError === undefined)
>-      aError = Ci.prplIAccount.ERROR_OTHER_ERROR;
>-    this._disconnect(aError, aException.toString());
>+  onError: function(aError = Ci.prplIAccount.ERROR_OTHER_ERROR,
>+                    aException = "") {
>+    this._disconnect(aError, aException);
>   },

Doesn't this change the behavior if aError is null? Wasn't there a reason for the explicit null check?

Comment 13

[Benedikt Pfeifer]

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

*** Original change on bio 1893 attmnt 2277 at 2013-06-05 07:58:54 UTC ***

I will need to look at this again, checking if there are indeed problems with the things that flo commented on.

Comment 14

[Patrick Cloke [:clokep]]

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

*** Original change on bio 1893 attmnt 2277 at 2013-10-11 17:32:16 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Benedikt Pfeifer]

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

*** Original change on bio 1893 attmnt 2277 at 2013-10-17 04:27:54 UTC ***

(In reply to comment #9)
> Comment on attachment 8354042 [details] [diff] [review] (bio-attmnt 2277) [details]
> Patch v3
> 
> >-function getTheme(aName)
> >+function getTheme(aName = Services.prefs.getCharPref(kEmoticonsThemePref))
> > {
> >-  let name = aName || Services.prefs.getCharPref(kEmoticonsThemePref);
> >+  let name = aName;
> 
> Why do we still need the |name| variable?

No, we do not.

> Also, is the error reporting correct if
> Services.prefs.getCharPref(kEmoticonsThemePref) throws?

Yes, it is. I've checked. The error is the same, only the line number changes according to the new position of the call.

> >   // CTCP messages are \001<COMMAND> [<parameters>]*\001.
> >-  sendCTCPMessage: function(aCommand, aParams, aTarget, aIsNotice) {
> >+  sendCTCPMessage: function(aCommand, aParams = [], aTarget = undefined,
> >+                            aIsNotice = false) {
> >     // Combine the CTCP command and parameters into the single IRC param.
> >     let ircParam = aCommand;
> >-    // If aParams is empty, then use an empty array. If aParams is not an array,
> >-    // consider it to be a single parameter and put it into an array.
> >-    let params = !aParams ? [] : Array.isArray(aParams) ? aParams : [aParams];
> >+    // If aParams is not an array, consider it to be a single parameter and put
> >+    // it into an array.
> >+    let params = Array.isArray(aParams) ? aParams : [aParams];
> >     if (params.length)
> >       ircParam += " " + params.join(" ");
> 
> Doesn't this change the behavior if aParams is null or ""? Are we sure this is
> OK?

It would change the behaviour as "" can be passed as parameter string (at least there's a test that does this: http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/test_ctcpQuote.js#57). The array would contain an empty string instead of actually being empty and the code would therefore run into the following if-block while it didn't before. We should revert this in my opinion, except if clokep knows why it would be OK anyways.

> >diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
> >   /* Called when there is an error in the xmpp session */
> >-  onError: function(aError, aException) {
> >-    if (aError === null || aError === undefined)
> >-      aError = Ci.prplIAccount.ERROR_OTHER_ERROR;
> >-    this._disconnect(aError, aException.toString());
> >+  onError: function(aError = Ci.prplIAccount.ERROR_OTHER_ERROR,
> >+                    aException = "") {
> >+    this._disconnect(aError, aException);
> >   },
> 
> Doesn't this change the behavior if aError is null? Wasn't there a reason for
> the explicit null check?

(In reply to comment #5)
> But did you check if we ever actually send null? I couldn't find any place that
> we do. I also think this is an abuse of an API and we should send undefined (or
> nothing at all). I actually greatly dislike the usage of onError throughout the
> XMPP code...

I couldn't find any call using "null" either.

Comment 16

[Patrick Cloke [:clokep]]

So as far as I can tell for this patch:
 - This cleans up the code a bit in some places, but in others I'm pretty meh on whether it's easier to read or not.
 - It seems like we don't like putting function calls as default parameters, although they do work.

Are these accurate? I can put up other patches that have this + the review comments above met.

Comment 17

[Florian Quèze [:florian]]

Yes.

Comment 18

[Patrick Cloke [:clokep]]

Attachment: IRC part, v4

So I kind of gave up on this a bit, but I liked a lot of the IRC changes, so I wanted to get those applied. This actually changes the sending CTCP message API cause...I realized the current API is insanely weird.

Comment 19

[Patrick Cloke [:clokep]]

Attachment: XMPP part, v4

I don't really have interest in pushing this part through, but I figured I should attach my WIP for XMPP.

Comment 20

[Patrick Cloke [:clokep]]

Attachment: IRC part, v5 [checked in, comment 22]

Previous version failed a test.

Comment 21

[aleth [:aleth]]

Comment on attachment 8418466 [details] [diff] [review]
IRC part, v5 [checked in, comment 22]

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

Thanks for making CTCP more consistent!

Comment 22

[Patrick Cloke [:clokep]]

(In reply to Patrick Cloke [:clokep] from comment #20)
> Created attachment 8418466 [details] [diff] [review]

https://hg.mozilla.org/comm-central/rev/da34b03c7e57

What do we want to do with this bug now?

Comment 23

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #22)
> (In reply to Patrick Cloke [:clokep] from comment #20)
> > Created attachment 8418466 [details] [diff] [review]
> 
> https://hg.mozilla.org/comm-central/rev/da34b03c7e57
> 
> What do we want to do with this bug now?

The XMPP WIP looks like a good (if trivial) change, why did you not r? on it? Then the bug could be closed.

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 8418460 [details] [diff] [review]
XMPP part, v4

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

r=me with the onError changes reverted.

::: chat/protocols/xmpp/xmpp.jsm
@@ -935,5 @@
>    },
>  
>    /* Called when there is an error in the xmpp session */
> -  onError: function(aError, aException) {
> -    if (aError === null || aError === undefined)

I'm afraid removing this can cause bugs. It's common to use "null" when calling a function to omit an optional parameter that isn't the parameter one.

@@ -937,5 @@
>    /* Called when there is an error in the xmpp session */
> -  onError: function(aError, aException) {
> -    if (aError === null || aError === undefined)
> -      aError = Ci.prplIAccount.ERROR_OTHER_ERROR;
> -    this._disconnect(aError, aException.toString());

I think the explicit .toString() here was because we expect to receive exception objects and don't want to propagate them further.

Comment 25

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #22)

> What do we want to do with this bug now?

What happened to comment 15 / attachment 8354042 [details] [diff] [review]? Was there nothing we wanted in that attachment?

Comment 26

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> (In reply to Patrick Cloke [:clokep] from comment #22)
> 
> > What do we want to do with this bug now?
> 
> What happened to comment 15 / attachment 8354042 [details] [diff] [review]?
> Was there nothing we wanted in that attachment?

A significant portion of that patch moved function calls into the default parameters list. There are probably small bits of it that we'd want though. I'll take another look.

Comment 27

[Patrick Cloke [:clokep]]

Attachment: XMPP part, v5

MPP part without the onError changes.

Comment 28

[Patrick Cloke [:clokep]]

Attachment: Other part, v4

Comment 29

[Florian Quèze [:florian]]

Comment on attachment 8425163 [details] [diff] [review]
Other part, v4

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

Looks good to me; not sure if you wanted Mic's review specifically (there's r=florian in the metadata of the patch).

Comment 30

[Patrick Cloke [:clokep]]

Comment on attachment 8425163 [details] [diff] [review]
Other part, v4

No, just trying to share the wealth. Thanks.

Comment 31

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/ae1820f9f3fa
https://hg.mozilla.org/comm-central/rev/a218c71265fa