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

RESOLVED FIXED in Firefox 42

Status

()

--
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ishikawa, Assigned: ishikawa)

Tracking

Trunk
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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)
(Assignee)

Comment 1

3 years ago
Created attachment 8626940 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → ishikawa
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
See Also: → bug 1167986
(Assignee)

Updated

3 years ago
Attachment #8626940 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8626940 - Flags: review?(mkmelin+mozilla) → feedback+
Component: General → Places
Product: Thunderbird → Toolkit
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Updated

3 years ago
Attachment #8626940 - Flags: review?(mak77)
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
Attachment #8626940 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
Created attachment 8629727 [details] [diff] [review]
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
Attachment #8626940 - Attachment is obsolete: true
Attachment #8629727 - Flags: review?(mak77)
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?
Attachment #8629727 - Flags: review?(mak77) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8630024 [details] [diff] [review]
fix incorrect use of built-in |arguments| function as an argument name (and a few more changes)  carrying over mak77=r+
Attachment #8629727 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8630024 - Flags: review+
(Assignee)

Comment 9

3 years ago
(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.)
(Assignee)

Comment 10

3 years ago
I have put 'checkin-needed' keyword.
TIA
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8e32a9f40ba
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.