277588 - the removeCommands method should accept the same array as used in defineCommands

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Glen Mailer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a4) Gecko/20040927
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a4) Gecko/20040927

The command manager accepts an 3d array for defineCommands but requires an array
of objects all with {name: command} which is superflous.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Glen Mailer]

Attachment: Fix for removeCommands

Comment 2

[Glen Mailer]

Attachment: corrected coding pedantics

Comment 3

[Robert Ginda]

actually, if we're being pedantic...

+        var command = isinstance(cmdary[i], Array) ?
+                      {name: cmdary[i][0]} : cmdary[i];

should be...

+        var command = isinstance(cmdary[i], Array) ?
+            {name: cmdary[i][0]} : cmdary[i];

Because the previous line has no unclosed paren or brace, the continuing line
gets a single indent level (4 spaces.)  Not a big offense though, nice patch.

I assume you figured out (or someone told you) that removeCommands was
originally meant to take an array of command objects, which happens to be the
return value of defineCommands.

Comment 4

[Glen Mailer]

(In reply to comment #3)
> actually, if we're being pedantic...
> 
> +        var command = isinstance(cmdary[i], Array) ?
> +                      {name: cmdary[i][0]} : cmdary[i];
> 
> should be...
> 
> +        var command = isinstance(cmdary[i], Array) ?
> +            {name: cmdary[i][0]} : cmdary[i];
> 
> Because the previous line has no unclosed paren or brace, the continuing line
> gets a single indent level (4 spaces.)  Not a big offense though, nice patch.
> 
> I assume you figured out (or someone told you) that removeCommands was
> originally meant to take an array of command objects, which happens to be the
> return value of defineCommands.

Heh, couldnt seem to find any examples of ternary indentation to go by.

Never really noticed about the returned array from defineCommands, is there any
point in using this path now then?

Comment 5

[Robert Ginda]

It's actually the same rule that determines what to do when you want to break
right before the right-hand-side of an assignment, as in...

http://lxr.mozilla.org/mozilla/source/extensions/irc/xul/content/static.js

I don't see any harm in keeping the old path around, probably someone is already
using it in a plugin.

Comment 6

[Robert Ginda]

aha, here's the other example I was looking for...

http://lxr.mozilla.org/mozilla/source/extensions/irc/xul/content/static.js#2082

Comment 7

[James Ross]

Comment on attachment 170699 [details] [diff] [review]
corrected coding pedantics

>Index: mozilla/extensions/irc/js/lib/command-manager.js
>-    for (var c in commands)
>-        this.removeCommand(commands[c]);
>+    for (var i = 0; i < cmdary.length; ++i)

This wont work when called using the previous style, which you very carefully
tried to cope with inside the loop.

http://lxr.mozilla.org/mozilla/source/extensions/irc/js/lib/command-manager.js#
179 sets a property for each command defined, using the command's name as the
property name. It is not an array - so it wont have a length or numeric
properties.

You should keep the |in| version of the loop, since arrays only have their
numeric indicies set as enumerable in normal circumstances, or somehow branch
the codepaths outside of the loop constructs.

Comment 8

[Glen Mailer]

Attachment: Fix v3

ok, finally got around to correcting that.

Comment 9

[Glen Mailer]

Attachment: fix v4

I really should double check my patches before i upload, missed a minor
pedantics fix.

Comment 10

[James Ross]

Comment on attachment 174910 [details] [diff] [review]
fix v4

r=[e-mail]

Comment 11

[James Ross]

Checked in --> FIXED.

In approximately 6 minutes a build with this fix will be available from:
  http://twpol.dyndns.org/mozilla/chatzilla/nightly/