Avoid using arguments.callee() and just give every function (expression) a name, in SeaMonkey

RESOLVED FIXED

Status

--
minor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=sgautherie][lang=js], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
https://developer.mozilla.org/en/JavaScript/Reference/Functions_and_function_scope/arguments/callee#Description
{
Note: You should avoid using arguments.callee() and just give every function (expression) a name.

Warning: The 5th edition of ECMAScript forbids use of arguments.callee() in strict mode.
}

***

"Found 101 matching lines in 38 files"

All in tests, except
{
/suite/common/src/nsSessionStore.js
    * line 408 -- aEvent.currentTarget.removeEventListener("load", arguments.callee, false);

/suite/common/aboutSessionRestore.js
    * line 135 -- newWindow.removeEventListener("load", arguments.callee, true);

/suite/common/utilityOverlay.js
    * line 345 -- toolbox.removeEventListener("beforecustomization", arguments.callee, false);
    * line 1533 -- browserWin.removeEventListener("pageshow", arguments.callee, true);
}

***

Bug 724331 comment 13
{
neil@parkwaycc.co.uk      2012-02-05 14:30:19 PST

No rush, we just need to change them as and when we patch those uses.
}

That seems to be a good G.F.B. to me...
It can be done in chunks, in blocking bugs, as need be.

NB: It may be worth fixing Firefox code first, when SeaMonkey copies it.
(Reporter)

Updated

7 years ago
Depends on: 726505, 726521
(Reporter)

Updated

7 years ago
Depends on: 735139
(Reporter)

Updated

7 years ago
Depends on: 735143
No longer depends on: 735139
(Assignee)

Comment 1

6 years ago
Created attachment 628341 [details] [diff] [review]
Replaced arguments.callee() with a name. (v1)

this patch also includes changes to the MPL headers.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #628341 - Flags: review?(neil)

Comment 2

6 years ago
> this patch also includes changes to the MPL headers.
Don't do that Gerv is about to land a tree wide change to the MPL headers.

Comment 3

6 years ago
Comment on attachment 628341 [details] [diff] [review]
Replaced arguments.callee() with a name. (v1)

>diff --git a/suite/common/aboutSessionRestore.js b/suite/common/aboutSessionRestore.js
[Don't need to bother with licence changes, Gerv will sort things out.]

>-    tab2.linkedBrowser.addEventListener("load", function(aEvent) {
>-      this.removeEventListener("load", arguments.callee, true);
>+    tab2.linkedBrowser.addEventListener("load", function anon1(aEvent) {
>+      this.removeEventListener("load", anon1, true);
Sorry, but I don't appreciate your choice of function names. Try to make up a meaningful, but not too long, name that would be useful in a stack trace. For instance, "tab2Loaded" could work well here. (Note that this is actually my third attempt at a function name, so feel free to improve on it.)
Attachment #628341 - Flags: review?(neil) → review-
(Assignee)

Comment 4

6 years ago
Created attachment 628617 [details] [diff] [review]
Replaced arguments.callee() with a name. (v2)
Attachment #628341 - Attachment is obsolete: true
Attachment #628617 - Flags: review?(neil)

Comment 5

6 years ago
Comment on attachment 628617 [details] [diff] [review]
Replaced arguments.callee() with a name. (v2)

>+  popup.addEventListener("popupshown", function triggerPopupPopupShown() {
>+    popup.removeEventListener("popupshown", triggerPopupPopupShown, false);
Nit: PopupPopup looks cumbersome. Just use triggerPopupShown.

>+      aSubject.addEventListener("load", function observeASubjectLoad(aEvent) {
>+        aEvent.currentTarget.removeEventListener("load", observeASubjectLoad, false);
I don't think it makes sense to include "observe" here, just use aSubjectLoad.

>+    duplicateTab.linkedBrowser.addEventListener("load",
>+                                            function testTab2DupLBLoad(aEvent) {
Nit: incorrect wrapping.
I don't think it's possible to wrap this nicely, so don't bother.

>   browser.addEventListener("load", function loadListener(e) {
>-    browser.removeEventListener("load", arguments.callee, true);
>+    browser.removeEventListener("load", loadListener, true);
...
>     newTab.addEventListener("SSTabRestored", function tabRestored(e) {
>-      newTab.removeEventListener("SSTabRestored", arguments.callee, true);
>+      newTab.removeEventListener("SSTabRestored", tabRestored, true);
Wow, how did those sneak past their original review ;-)
Attachment #628617 - Flags: review?(neil) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 629024 [details] [diff] [review]
Replaced arguments.callee() with a name. (v3)

Fixed nits from comment #5.
Attachment #628617 - Attachment is obsolete: true
Attachment #629024 - Flags: review+
(Assignee)

Comment 7

6 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/869e0e8618c3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.