Closed
Bug 724441
Opened 13 years ago
Closed 13 years ago
Avoid using arguments.callee() and just give every function (expression) a name, in SeaMonkey
Categories
(SeaMonkey :: General, defect)
SeaMonkey
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: sgautherie, Assigned: ewong)
References
()
Details
(Whiteboard: [good first bug][mentor=sgautherie][lang=js])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
ewong
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Reporter | ||
Updated•13 years ago
|
Comment 2•13 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•13 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•13 years ago
|
||
Attachment #628341 -
Attachment is obsolete: true
Attachment #628617 -
Flags: review?(neil)
Comment 5•13 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•13 years ago
|
||
Fixed nits from comment #5.
Attachment #628617 -
Attachment is obsolete: true
Attachment #629024 -
Flags: review+
Assignee | ||
Comment 7•13 years ago
|
||
Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/869e0e8618c3
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•