1178041 - SyntaxError: redefining arguments is deprecated _tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_signed_migrate.js

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[ISHIKAWA, Chiaki]

SyntaxError: redefining arguments is deprecated _tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_signed_migrate.js

I saw a few strange errors coming from test_signed_migrate.js when I
ran |make xpchsell-tests| of C-C TB.  This message is very new.

      2 "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "SyntaxError: redefining arguments is deprecated" {file: "/NREF-COMM-CENTRAL/objdir-tb3/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_signed_migrate.js" line: 30 column: 52 source: "  openWindow: function(parent, url, name, features, arguments) {
      1 PROCESS | 14289 | JavaScript strict warning: /NREF-COMM-CENTRAL/objdir-tb3/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_signed_migrate.js, line 30: SyntaxError: redefining arguments is deprecated
      1 PROCESS | 14181 | JavaScript strict warning: /NREF-COMM-CENTRAL/objdir-tb3/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_signed_migrate.js, line 30: SyntaxError: redefining arguments is deprecated


30   openWindow: function(parent, url, name, features, arguments) {
31     let ids = arguments.QueryInterface(AM_Ci.nsIVariant);
32     this.sawAddon = ids.indexOf(ID) >= 0;
33   

As it turns out this code uses an argument |arguments|.
The name collides with the built-in function to pass
the arguments of a function inside its body.
Thus JS interpreter/compiler complains.

Attached patch should fix the issue.

PS: I wonder if this has anything to do with Bug 1167986
	
TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_signed_install.js (and signing failures)

Comment 1

[ISHIKAWA, Chiaki]

Attachment: fix incorrect use of built-in |arguments| function as an argument name

This patch should fix the use of the buitl-in function name
|arguments| as a function parameter.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 8626940 [details] [diff] [review]
fix incorrect use of built-in |arguments| function as an argument name

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

Looks good to me, but you need a toolkit/ reviewer for this.

Comment 3

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin from comment #2)
> Comment on attachment 8626940 [details] [diff] [review]
> fix incorrect use of built-in |arguments| function as an argument name
> 
> Review of attachment 8626940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, but you need a toolkit/ reviewer for this.

Thank you for the comment. Now I will look up a toolkit reviewer. I hope recommendation feature works.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8626940 [details] [diff] [review]
fix incorrect use of built-in |arguments| function as an argument name

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

Since here, would you mind fixing the other tests doing the same thing too?

http://mxr.mozilla.org/mozilla-central/search?string=openWindow%3A+%23arguments&regexp=on

::: toolkit/mozapps/extensions/test/xpcshell/test_signed_migrate.js
@@ +26,5 @@
>  // Override the window watcher
>  var WindowWatcher = {
>    sawAddon: false,
>  
> +  openWindow: function(parent, url, name, features, arg) {

nit: maybe plural args would be clearer

Comment 5

[ISHIKAWA, Chiaki]

(In reply to Marco Bonardo [::mak] from comment #4)
> Comment on attachment 8626940 [details] [diff] [review]
> fix incorrect use of built-in |arguments| function as an argument name
> 
> Review of attachment 8626940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since here, would you mind fixing the other tests doing the same thing too?
> 
> http://mxr.mozilla.org/mozilla-central/
> search?string=openWindow%3A+%23arguments&regexp=on
> 

I have no idea why I did not notice this. Maybe these errors would appear later, or maybe because I was looking at C-C TB's xpcshell tests.

> ::: toolkit/mozapps/extensions/test/xpcshell/test_signed_migrate.js
> @@ +26,5 @@
> >  // Override the window watcher
> >  var WindowWatcher = {
> >    sawAddon: false,
> >  
> > +  openWindow: function(parent, url, name, features, arg) {
> 
> nit: maybe plural args would be clearer

|args| seems better, so I will roll up a patch that addresses all the files.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: fix incorrect use of built-in |arguments| function as an argument name (and a few more changes)

Additional changes.

I notice a few more usage of |arguments| as member field and changes them, too, for consistency.

TIA

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8629727 [details] [diff] [review]
fix incorrect use of built-in |arguments| function as an argument name (and a few more changes)

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

::: toolkit/mozapps/extensions/test/xpcshell/test_metadata_update.js
@@ +37,5 @@
>  
>  // This will be called to show the compatibility update dialog.
>  var WindowWatcher = {
>    expected: false,
> +  args: null,

looks like this is unused and could just be removed?

Comment 8

[ISHIKAWA, Chiaki]

Attachment: fix incorrect use of built-in |arguments| function as an argument name (and a few more changes) carrying over mak77=r+

Comment 9

[ISHIKAWA, Chiaki]

(In reply to Marco Bonardo [::mak] from comment #7)
> Comment on attachment 8629727 [details] [diff] [review]
> fix incorrect use of built-in |arguments| function as an argument name (and
> a few more changes)
> 
> Review of attachment 8629727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/test/xpcshell/test_metadata_update.js
> @@ +37,5 @@
> >  
> >  // This will be called to show the compatibility update dialog.
> >  var WindowWatcher = {
> >    expected: false,
> > +  args: null,
> 
> looks like this is unused and could just be removed?

Yes, I found that this particular |argument| aka |args| is not used in the code and so took it out.
Rrefreshed the patch, carrying over your r+.

Can I put in the checkin-needed keyword?
(Yes, I tested it locally.)

Comment 10

[ISHIKAWA, Chiaki]

I have put 'checkin-needed' keyword.
TIA

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a8e32a9f40ba

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8e32a9f40ba