Closed
Bug 296662
Opened 19 years ago
Closed 19 years ago
Javascript strict warnings in preferences dialog
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox1.5
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(4 obsolete files)
There are a lot of strict warnings in different tabs of the preferences dialog. A patch will be submitted within the next days. The most of them contain such a description: Warning: reference to undefined property aElement.value Source File: chrome://global/content/bindings/preferences.xml Line: 355 or Warning: trailing comma is not legal in ECMA-262 object initializers Source File: chrome://mozapps/content/downloads/DownloadProgressListener.js Line: 241 Source Code: };
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Summary: Javascript strict warnings in preferences dialog → Javascript strict warnings in preferences dialog
Assignee | ||
Comment 1•19 years ago
|
||
Assignee: nobody → hskupin
Assignee | ||
Updated•19 years ago
|
Attachment #185394 -
Flags: review?(benjamin)
Comment 2•19 years ago
|
||
Comment on attachment 185394 [details] [diff] [review] Patch Just a few nits: - Most of the files in this patch don't really belong together, all they have in common are strict warnings. This will frighten off shy reviewers. ;-) >Index: toolkit/components/passwordmgr/resources/content/passwordManager.js >=================================================================== [...] >@@ -498,23 +498,23 @@ function DeleteSelectedItemFromTree > } > } > > box.endUpdateBatch(); > > // update selection and/or buttons >- var removeButton = document.getElementById(removeButton); >- var removeAllButton = document.getElementById(removeAllButton); >+ var removeBtn = document.getElementById("removeButton"); >+ var removeAllBtn = document.getElementById("removeAllButton"); Since removeButton is a parameter of DeleteSelectedItemFromTree, it's plain wrong to try to find "removeButton". But the real problem with this code is a different one, anyway: The Mozilla coding style guide requires/suggests that function parameters are prefixed by an 'a'. So a better patch for the problem in this funtion would be renaming the parameters, eg. removeAllButton -> aRemoveAllButton. >Index: toolkit/mozapps/downloads/content/downloads.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v >retrieving revision 1.43 >diff -p -u -6 -r1.43 downloads.js >--- toolkit/mozapps/downloads/content/downloads.js 12 Apr 2005 15:55:49 -0000 1.43 >+++ toolkit/mozapps/downloads/content/downloads.js 5 Jun 2005 14:04:55 -0000 >@@ -643,16 +643,16 @@ function buildContextMenu(aEvent) > idx = 0; > > var menus = gContextMenus[idx]; > for (var i = 0; i < menus.length; ++i) > popup.appendChild(document.getElementById(menus[i]).cloneNode(true)); > >- return true; >+ return; > } > >- return false; >+ return; > } onpopupshowing uses "return buildContextMenu(aEvent)", so removing the return values is just wrong. >@@ -781,13 +781,13 @@ function initAutoDownloadDisplay() > return dir; > } > > var displayName = null; > switch (pref.getIntPref("browser.download.folderList")) { > case 0: >- folder = getDownloadsFolder("Desktop"); >+ var folder = getDownloadsFolder("Desktop"); > var strings = document.getElementById("downloadStrings"); > displayName = strings.getString("displayNameDesktop"); > break; > case 1: > folder = getDownloadsFolder("Downloads"); [...] } if (folder) { Even though JS permits such definitions (all variables defined in a function are on function level regradless how deeply nested in {} they are), it's bad style to define a variable on a deeper level than its usage. >Index: browser/components/preferences/cookies.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/preferences/cookies.js,v >retrieving revision 1.6 >diff -p -u -6 -r1.6 cookies.js >--- browser/components/preferences/cookies.js 9 Mar 2005 03:33:42 -0000 1.6 >+++ browser/components/preferences/cookies.js 5 Jun 2005 14:04:59 -0000 >@@ -478,18 +478,18 @@ var gCookiesWindow = { > { > var seln = this._view.selection; > var ids = ["nameLabel", "name", "valueLabel", "value", "isDomain", "host", > "pathLabel", "path", "isSecureLabel", "isSecure", "expiresLabel", > "expires"]; > if (aItem && !aItem.container && seln.count > 0) { >- properties = { name: aItem.name, value: aItem.value, host: aItem.host, >- path: aItem.path, expires: this.formatExpiresString(aItem.expires), >- isDomain: aItem.isDomain ? this._bundle.getString("domainColon") >- : this._bundle.getString("hostColon"), >- isSecure: aItem.isSecure ? this._bundle.getString("forSecureOnly") >- : this._bundle.getString("forAnyConnection") }; >+ var properties = { name: aItem.name, value: aItem.value, host: aItem.host, See above. The 'var properties' definition is too deep, move it onto the 'var ids' level.
Attachment #185394 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 3•19 years ago
|
||
Ok, I've updated the patch to your mentioned list of things to do. For the future I'll take care about not mixing different files within one patch. Thanks.
Attachment #185394 -
Attachment is obsolete: true
Comment 4•19 years ago
|
||
Comment on attachment 185398 [details] [diff] [review] Patch v2 Looks good now (to me ;-) ).
Assignee | ||
Updated•19 years ago
|
Attachment #185398 -
Flags: review?(benjamin)
Comment 5•19 years ago
|
||
Comment on attachment 185398 [details] [diff] [review] Patch v2 I dislike the gratuitous variable renaming in passwordManager.js: please remove that and r=me
Attachment #185398 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 6•19 years ago
|
||
> I dislike the gratuitous variable renaming in passwordManager.js: please
> remove that and r=me
Thank you Benjamin. I reverted passwordManager.js to my changes of the first
applied patch. So I take-over your r.
Attachment #185398 -
Attachment is obsolete: true
Attachment #185474 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #185474 -
Flags: approval-aviary1.1a2?
Assignee | ||
Comment 7•19 years ago
|
||
Patch without update.js which is covered in bug 291672 with some more improvements as I made here.
Attachment #185474 -
Attachment is obsolete: true
Attachment #185527 -
Flags: review+
Attachment #185527 -
Flags: approval-aviary1.1a2?
Comment 8•19 years ago
|
||
Comment on attachment 185527 [details] [diff] [review] Patch without update.js [Checked in: Comment 13] >Index: toolkit/content/widgets/preferences.xml >+ var value = ("value" in aElement) ? aElement.value : aElement.getAttribute("value"); Can you explain this change? It looks kind of suspicious to me. Isn't the property mapped to the attribute? >Index: toolkit/components/passwordmgr/resources/content/passwordManager.js >- var removeButton = document.getElementById(removeButton); >- var removeAllButton = document.getElementById(removeAllButton); >+ var removeBtn = document.getElementById(removeButton); >+ var removeAllBtn = document.getElementById(removeAllButton); Benjamin asked you to remove this renaming from the patch. It's generally not a good idea to bring over review markings if the patch doesn't address the issues or if non trivial changes were made to that patch.
Attachment #185527 -
Flags: review+
Attachment #185527 -
Flags: approval-aviary1.1a2?
Updated•19 years ago
|
Attachment #185474 -
Flags: review+
Attachment #185474 -
Flags: approval-aviary1.1a2?
Comment 9•19 years ago
|
||
> >Index: toolkit/components/passwordmgr/resources/content/passwordManager.js > > >- var removeButton = document.getElementById(removeButton); > >- var removeAllButton = document.getElementById(removeAllButton); > >+ var removeBtn = document.getElementById(removeButton); > >+ var removeAllBtn = document.getElementById(removeAllButton); > > Benjamin asked you to remove this renaming from the patch. No, he didn't. > It's generally not a good idea to bring over review markings if the patch > doesn't address the issues or if non trivial changes were made to that patch. It's generally not a good idea to talk about things one doesn't understand. ;-) The line var removeButton = document.getElementById(removeButton); *needs* to have one variable renamed, else it will lead to JS strict warning. My advice (still!) is to follow the Mozilla naming guidelines and correct the problem by changing the argument names - but bsmedberg rejected that (for whatever reason). So Henrik *had* to rename the variable names...
Comment 10•19 years ago
|
||
(In reply to comment #9) > No, he didn't. I don't see how else you could interpret comment 5, but I suppose I may have misunderstood what he meant. > It's generally not a good idea to talk about things one doesn't understand. ;-) > The line > var removeButton = document.getElementById(removeButton); > *needs* to have one variable renamed, else it will lead to JS strict warning. Which JS strict warning? The variable redeclaration warning that was recently removed in bug 291868? That bug along with Benjamin's comment was what led me to suggest the change was unnecessary. In any case, it doesn't really matter, and I would tend to agree with you anyways, but asking for another review on a changed patch never hurts.
Assignee | ||
Comment 11•19 years ago
|
||
> Which JS strict warning? The variable redeclaration warning that was recently
> removed in bug 291868?
There is no redeclaration warning. The additional declared variables hides the
function parameters removeButton and removeAllButton, e.g.
Warning: variable removeAllButton hides argument
Source File: chrome://passwordmgr/content/passwordManager.js
Line: 466, Column: 6
Source Code:
var removeAllButton = document.getElementById(removeAllButton);
Assignee | ||
Updated•19 years ago
|
Attachment #185527 -
Flags: review?(benjamin)
Assignee | ||
Comment 12•19 years ago
|
||
(In reply to comment #8) > >+ var value = ("value" in aElement) ? aElement.value : aElement.getAttribute("value"); > > Can you explain this change? It looks kind of suspicious to me. Isn't the > property mapped to the attribute? The value for the element is set here: http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/preferences.xml#339 So we could have a better solution?
Updated•19 years ago
|
Attachment #185527 -
Flags: review?(benjamin) → review+
Updated•19 years ago
|
Attachment #185527 -
Flags: approval-aviary1.1a2?
Updated•19 years ago
|
Attachment #185527 -
Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Updated•19 years ago
|
Whiteboard: [checkin needed]
Comment 13•19 years ago
|
||
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #185527 -
Attachment description: Patch without update.js → Patch without update.js
[Checked in: Comment 13]
Attachment #185527 -
Attachment is obsolete: true
Updated•19 years ago
|
Whiteboard: [checkin needed]
Target Milestone: --- → Firefox1.1
Version: unspecified → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•