759446 - Use rest arguments where possible

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We use functions that take variable numbers of arguments a fair bit in the add-ons manager code and currently use the arguments pseudo-array to handle them. JS just got full support though in the form of rest arguments (see the URL) so we should look at switching to that.

A quick list of places that might benefit from this: http://bit.ly/JyPQAd

Comment 1

[Luis Ares]

Can I be of any help? This is my first time trying to contribute here, and I thought this would be a good chance for me to learn how the process of fixing a bug works. 

I created a patch which changed all the uses of "Array.slice(arguments" given in an earlier comment. Let me know if I should attach it to this bug.

(In reply to Dave Townsend (:Mossop) from comment #0)
> We use functions that take variable numbers of arguments a fair bit in the
> add-ons manager code and currently use the arguments pseudo-array to handle
> them. JS just got full support though in the form of rest arguments (see the
> URL) so we should look at switching to that.
> 
> A quick list of places that might benefit from this: http://bit.ly/JyPQAd

Comment 2

[Tony Mechelynck [:tonymec]]

(In reply to duelistone from comment #1)
[...]
> I created a patch which changed all the uses of "Array.slice(arguments"
> given in an earlier comment. Let me know if I should attach it to this bug.
[...]
Short answer: Yes. You should also ASSIGN the bug to yourself, then get someone to review your patch (normally an owner or peer of the concerned module).

Long answer: See https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch

Comment 3

[Luis Ares]

Attachment: Patch replacing some uses of the arguments pseudo-array with rest arguments

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 633858 [details] [diff] [review]
Patch replacing some uses of the arguments pseudo-array with rest arguments

Great, thanks :) Will look at this later today.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 633858 [details] [diff] [review]
Patch replacing some uses of the arguments pseudo-array with rest arguments

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

Looking good :)

I noticed there are a few other cases where we use 'arguments', but without Array.slice():
* Using Array.splice() instead: http://is.gd/bIjR4E
* In callInstallListeners() and callAddonListeners() here: http://is.gd/R8rAAg (Might have to remove aMethod from the argument list - it's nice to have, but only there for self-documentation purposes).

Also, this isn't necessary, but bonus points if you rename the 'args' variables to 'aArgs', to match with the style of prefixing function arguments with 'a'.

::: toolkit/mozapps/extensions/test/browser/head.js
@@ +375,2 @@
>    var bundle = Services.strings.createBundle("chrome://mozapps/locale/extensions/extensions.properties");
>    if (arguments.length == 1)

This should be args.length now.

Comment 6

[Luis Ares]

Thanks for the review. I'm going to look over it soon.

Comment 7

[Luis Ares]

Attachment: rest_arguments_patch

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 634163 [details] [diff] [review]
rest_arguments_patch

When you attach a new patch, you can set older patches to be "obsolete" (makes it clear which patches are now old), and also set the review flag to "?" and enter the email of who should review it (to make sure that person knows, otherwise it's easy to miss).

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 634163 [details] [diff] [review]
rest_arguments_patch

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

Excellent - thanks :)

I'll get this landed for you.

Comment 10

[Luis Ares]

I'll keep your tips in mind for next time I submit a patch. Glad I could be of help.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, before I land your patch, what name would you like to be used as the patch author?

Comment 12

[Luis Ares]

The name which shows up as my username now (namely "Luis Ares").

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Landed on the fx-team branch, which gets merged into mozilla-central roughly every day:
https://hg.mozilla.org/integration/fx-team/rev/92d3a40382aa

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/92d3a40382aa