Closed Bug 249012 Opened 20 years ago Closed 19 years ago

tons of strict Javascript warnings in EM (137041)

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED WORKSFORME

People

(Reporter: bugzilla, Assigned: bugs)

References

Details

Attachments

(1 file, 4 obsolete files)

Warning: redeclaration of var i Source File: file:///C:/Program%20Files/Mozilla/Releases/Mozilla%20Firefox/components/nsExtensionManager.js Line: 524, Column: 13 Source Code: for (var i = 0; i < lines.length; ++i) Warning: redeclaration of var isProfile Source File: file:///C:/Program%20Files/Mozilla/Releases/Mozilla%20Firefox/components/nsExtensionManager.js Line: 555, Column: 10 Source Code: var isProfile = parts[1] == this.TOKEN_PROFILE; Warning: redeclaration of var i Source File: file:///C:/Program%20Files/Mozilla/Releases/Mozilla%20Firefox/components/nsExtensionManager.js Line: 1007, Column: 15 Source Code: for (var i = 0; i < this._packagesForExtension.length; ++i) { etc etc etc etc... do you want a patch to fix these?
*** Bug 249235 has been marked as a duplicate of this bug. ***
Henrik, I'd love to see this fixed. Are you going to make a patch?
I dont have the time. Sorry. It should be very easy to fix those warnings. Ben: Do you want to fix those yourself or do you want a patch?
*** Bug 254130 has been marked as a duplicate of this bug. ***
Great, please use a better summary to circumvent dupes. This one can't find anybody. :(
Summary: tons of strict Javascript warnings in EM → tons of strict Javascript warnings in EM (nsExtensionManager.js)
I modified several Javascipt files in FireFox in order to prevent these strict warnings. The modified files are in a zip file along with a readme file. Please let FireFox devs always use strict javascript checking while developing ff components. This should prevent these kind of errors and is an excellent early warning system for other types of bugs.
phendriks@hotmail.com: please use cvs diff -up instead.
Comment on attachment 159321 [details] Modified javascripts for Firefox components I'm sorry, but you need to modify the files from the source code. You've taken the code from browser.jar and toolkit.jar, but that code is pre-processed. The original source code contains a lot of #ifdef, which are splitting the code between platforms and apps. For example, your code doesn't contain this large block, because it is for Thunderbird only: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/ content/extensions.js&rev=1.6.6.23&mark=831-886#831 This document tells you how to get the source: http://www.mozilla.org/cvs.html But also read this document, especially the note (which I just added) about the aviary branch: http://www.mozilla.org/projects/firefox/build.html
Attachment #159321 - Attachment is obsolete: true
Blocks: 263648
Comment on attachment 162183 [details] [diff] [review] nsExtensionManager.js.in patch: fix 29 warnings OK, this fixes the warnings. But please declare variables outside if/else and switch/case clauses, 'for' loops etc., instead of just removing all "var"s after the first one. That's both clearer and less likely to become wrong because of future changes. And please remove the "var"s instead of commenting them out.
Renamed some vars so intended scope is still clear.
Attachment #162183 - Attachment is obsolete: true
Why do you introduce a bunch of new variables lke i1, i2, i3, PRBool1, PRBool2, win1, win2, etc., especially if they are not used outside their for-loop or the same values are assigned to them? For example, why don't you do this: var i; switch... case...: for (i=0... case...: for (i=0...
To make clear the intended scope of each variable's use, so the code easier to understand and maintain. In cases where there were a series of for loops close together, the patched code does declare one variable i and reuse it. However, many of the functions are long, with large if-else blocks or switch cases. Declaring a variable in a nested block makes clear that a variable is new and used only within that block. People reading or modifying the code can quickly see that: * The variable is new, so there's no need to think about what the previous value was, and when it is safe to overwrite it. * The value is only used in the block, so there's no need to think about whether the variable is read later outside the block. The previous patch preserved the intended declaration block by commenting the reused /*var*/. This patch preserves the intended declaration block by giving the conflicting vars different names in different branches.
Can you attach a correct nsExtensionManager.js file (already patched) for us to replace the existing one on our releases? This errors really annoy me because I have installed the WebDeveloper extension and configured it to pop up the JS Console whenever a warning/error is detected. Thanks.
OS: Windows XP → All
Hardware: PC → All
Attached patch patch (obsolete) — Splinter Review
This patch only creates one new variable, appExtensionsVersion in GetTarget (the appVersion variable was being used for two purposes in that function).
Attachment #162933 - Attachment is obsolete: true
Attachment #172829 - Flags: review?(bugs)
Attachment #172829 - Flags: review?(bugs) → review?(mconnor)
Comment on attachment 172829 [details] [diff] [review] patch this is going to be rather far down my list, so you may want to look at the current list of toolkit peers and find another reviewer, since its probably 3 weeks at the earliest before I'll have real time for reviews.
Thanks for replying so quickly, Mike. Where can I find that list? (this is my first patch)
Attachment #172829 - Flags: review?(mconnor) → review?(darin)
so why are we concerned with these javascript script warnings? i profess to be ignorant about what the significance of these warnings are. if this is important, then maybe someone should include this in the build system so that each JS file is checked during a build. that way we can keep the tree "clean" if that is indeed important.
The first two could be bad, and should be fixed. Look at http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#320 and http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#323 and you'll see the problem. The rest are about redeclared vars, which generally are less of a problem. It would be tidier to declare once per name in each top-level or function scope, but it's not a big deal. /be
perhaps the patch could also remove the dump functions
I care about them because JS strict warnings are one of the few diagnostic tools I have in extension/XBL/etc.-land, but enabling them treats me to an enormous amount of spew in the JS console, obscuring the ones I'm looking for (such as my own!). The same reason that I care about warnings in C++, really.
Comment on attachment 172829 [details] [diff] [review] patch >Index: nsExtensionManager.js.in >- var item = this._getResourceForItem(aItemID); >+ //var item = this._getResourceForItem(aItemID); > var index = container.IndexOf(item); Why did you move this, since it is used in the next line?
The previous version of the patch explained better: it is bound to the same thing a few lines above. https://bugzilla.mozilla.org/attachment.cgi?id=162933&action=diff#mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in_sec11 (In reply to comment #23) > (From update of attachment 172829 [details] [diff] [review] [edit]) > >Index: nsExtensionManager.js.in > > >- var item = this._getResourceForItem(aItemID); > >+ //var item = this._getResourceForItem(aItemID); > > var index = container.IndexOf(item); > > Why did you move this, since it is used in the next line? >
Attachment #172829 - Flags: review?(darin) → review+
Thanks for the review. Can you check this in?
(In reply to comment #19) > so why are we concerned with these javascript script warnings? i profess to be > ignorant about what the significance of these warnings are. if this is > important, then maybe someone should include this in the build system so that > each JS file is checked during a build. that way we can keep the tree "clean" > if that is indeed important. I think this is a fine idea. In general, it seems to me that --enable-debug ought to force JS strict warnings to be on, so that people will see them.
The last patch skips the first 2 warnings about == instead of = in stackTraceFunctionFormat. Overlooked?
(In reply to comment #27) > The last patch skips the first 2 warnings about == instead of = in > stackTraceFunctionFormat. Overlooked? That function is commented out. http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#318
what would it take to get JS strict warnings enabled by default in debug builds? or would that just be too painful?
Just an #ifdef in all.js -- very simple, in fact. I don't know the pain level.
darin: you mean like bug 248528?
Flags: blocking-aviary1.1?
Summary: tons of strict Javascript warnings in EM (nsExtensionManager.js) → tons of strict Javascript warnings in EM (137041)
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Comment on attachment 172829 [details] [diff] [review] patch [Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE) Could someone check this in ? >Index: nsExtensionManager.js.in >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v >@@ -3992,7 +4001,7 @@ nsExtensionsDataSource.prototype = { > .createInstance(Components.interfaces.nsIRDFContainer); > container.Init(ds, extensions); > >- var item = this._getResourceForItem(aItemID); >+ //var item = this._getResourceForItem(aItemID); > var index = container.IndexOf(item); > if (index > 1) { > container.RemoveElement(item, false); This should be removed, instead of commented out. >@@ -4010,7 +4019,7 @@ nsExtensionsDataSource.prototype = { > .createInstance(Components.interfaces.nsIRDFContainer); > container.Init(ds, extensions); > >- var item = this._getResourceForItem(aItemID); >+ item = this._getResourceForItem(aItemID); > var index = container.IndexOf(item); > var count = container.GetCount(); > if (index < count) { This should be removed, instead of modified.
Attachment #172829 - Flags: approval-aviary1.0.1?
Version: unspecified → Trunk
Comment on attachment 172829 [details] [diff] [review] patch 1.0.1 is a security update release, and Firefox 1.0.1 has already shipped
Attachment #172829 - Flags: approval-aviary1.0.1? → approval-aviary1.0.1-
Benjamin, do you agree with my comment 32 which answers your comment 23 ?
I'm this close to removing the strict warning for redeclaration of var as var. It might flag confused attempts to use block locals in JS, but more likely, it just whines about localized var redeclaration that makes code more readable and makes the parts of a larger function stand alone better. I've talked to Ben about this, and we agree that such warnings should not be "fixed" by minimizing var declarations to the earliest point in the file, at least not without module owner agreement. I don't think that's forthcoming here, so this bug may be WONTFIX. I hope the few more serious warnings were fixed. /be
(In reply to comment #35) > I'm this close to removing the strict warning for redeclaration of var as var. > It might flag confused attempts to use block locals in JS, but more likely, it > just whines about localized var redeclaration that makes code more readable and > makes the parts of a larger function stand alone better. Well, I "like" the warnings as the language seems to work that way... :-| > I've talked to Ben about this, and we agree that such warnings should not be > "fixed" by minimizing var declarations to the earliest point in the file, at > least not without module owner agreement. I don't think that's forthcoming > here, so this bug may be WONTFIX. Not without owner agreement, agreed. (Why would module owner agreement not come on this patch ?) > I hope the few more serious warnings were fixed. (my 2 cts)
(In reply to comment #35) > I'm this close to removing the strict warning for redeclaration of var as var. > > I hope the few more serious warnings were fixed. (Bug 291868 was fixed ... Time to revive the current one !?)
Comment on attachment 172829 [details] [diff] [review] patch The variable redeclaration strict warning has just been removed by bug 291868, so we can concentrate on the more severe warnings, if any.
Attachment #172829 - Attachment is obsolete: true
I think we should close this bug. There are other ones for the remaining strict warnings: bug 291371 and bug 291265.
Flags: blocking-aviary1.1+ → blocking-aviary1.1-
Per comment 39, closing. /be
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: