60630 - javascript strict warnings in pref-charset.js

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect, P3)

Description

[Henrik Gemal]

JavaScript strict warning:
chrome://communicator/content/pref/pref-charset.js line 150: assignment to 
undeclared variable j

JavaScript strict warning:
chrome://communicator/content/pref/pref-charset.js line 188: assignment to 
undeclared variable arrayOfPrefs

JavaScript strict warning:
chrome://communicator/content/pref/pref-charset.js line 195: assignment to 
undeclared variable i

JavaScript strict warning:
chrome://communicator/content/pref/pref-charset.js line 197: assignment to 
undeclared variable str

JavaScript strict warning:
chrome://communicator/content/pref/pref-charset.js line 93: assignment to 
undeclared variable atom

JavaScript strict warning:
chrome://communicator/content/pref/pref-charset.js line 99: assignment to 
undeclared variable tit

JavaScript strict warning:
chrome://communicator/content/pref/pref-charset.js line 105: assignment to 
undeclared variable visible

Comment 1

[Henrik Gemal]

Attachment: Patch to fix javascript strict warnings

Comment 2

[scottputterman]

reassigning to nhotta.

Comment 3

[nhottanscp]

r=nhotta

Comment 4

[Mark Olson]

Attachment: patch - correct global and more aggressive cleanup

Comment 5

[Mark Olson]

I was looking at the previous patch and noticed it missed a case or two... Also
the atom variable must be declared global for the code to work.

This new patch also includes an agressive whitespace rearranging as well as \t
and dump() removal, since debugging was rather difficult with some of the
non-standard formatting in the file.  -wu diff on request.

Comment 6

[nhottanscp]

Reassign to [e-mail], r=nhotta, please get sr and check in. Let me
know if you want me to checkin.
Please test "Character Coding -> Customize..." in three cases, browser, mail
compose and mail view. Test the dialog functionality, also check the change in
the dialog actually changes the charset menu items.

Comment 7

[Mark Olson]

Ok, tested a change in each of the 3 windows, worked great.  All open windows
noticed the change.

I noticed 2 (existing) nits about the dialog though - Double click doesn't move
items, and when focus is in on list, it takes 2 clicks to gain focus in the
other.  These are both minor, but another bug could be filed to clean those up.

Comment 8

[nhottanscp]

bug 65973 - Customize Character Coding dialog needs UI fixup

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

couple of comments:

I think literals are nicer than constructing with 'new', e.g.

var foopy = []; instead of var foopy = new Array();
var noopy = ""; instead of var foopy = new String();

but that's stylistic. 

I see you're dumping to the console in some catch blocks. What is the benefit of 
this? If you want to convey useful information for debugging, then leave out 
try..catch altogether and let the exception be thrown. But in the normal case if 
an exception is thrown as part of normal operation, you probably just want to 
stifle, and the dump is meaningless as optimized clients run without consoles. 

Comment 10

[Mark Olson]

Attachment: patch - add removal of most try{} blocks, new String()/Array()

Comment 11

[Mark Olson]

Attachment: (real file this time)

Comment 12

[Mark Olson]

As it turned out, the pass to remove try{}'s where appropriate resulted in
removing quite a few lines of code that were just wrong (the catch{} was
executing every time).  The three try{}'s left should be legitimate, and do not
dump() on failure.

nhotta, you may want to look at this for r= reconsideration, the updates are
non-trivial.

Comment 13

[nhottanscp]

r=nhotta

Comment 14

[Mark Olson]

cc'ing alecf for sr

Comment 15

[Alec Flett]

this is a big review, I'll try to get to it after moz 0.8

Comment 16

[Alec Flett]

thanks for cleaning this up! what a mess
sr=alecf

Comment 17

[Peter "jag" Annema]

Checked this in for mao. Marking fixed.

Comment 18

[[e-mail] (gone - use [e-mail] instead)]

This isn't mail/news, over to the browser team to verify.

Comment 19

[Claudius Gayle]

hot potato...sairuh i think the 1-24 comments are a clue for repro/testing

Comment 20

[Henrik Gemal]

I cant see the warnings with todays build 20010619.
Will reopen if I see them again.

Please add the following line to your prefs.js file, so we could avoid all 
these strict warning fixup...:
user_pref("javascript.options.strict", true);