296662 - Javascript strict warnings in preferences dialog

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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:
};

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch

Comment 2

[Karsten Düsterloh]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v2

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.

Comment 4

[Karsten Düsterloh]

Comment on attachment 185398 [details] [diff] [review]
Patch v2

Looks good now (to me ;-) ).

Comment 5

[Benjamin Smedberg]

Comment on attachment 185398 [details] [diff] [review]
Patch v2

I dislike the gratuitous variable renaming in passwordManager.js: please remove
that and r=me

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Final patch

> 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.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch without update.js [Checked in: Comment 13]

Patch without update.js which is covered in bug 291672 with some more
improvements as I made here.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 9

[Karsten Düsterloh]

> >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

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

> 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);

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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?

Comment 13

[Daniel Brooks [:db48x]]

fix checked in

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

v