58694 - javascript strict warnings in filepicker.js

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Jarrod Gray]

JavaScript strict warning: 
chrome://global/content/filepicker.js line 168: redeclaration of var j

JavaScript strict warning: 
chrome://global/content/filepicker.js line 190: redeclaration of var ruleNode

JavaScript strict warning: 
chrome://global/content/filepicker.js line 400: redeclaration of var i

JavaScript strict warning: 
chrome://global/content/filepicker.js line 403: redeclaration of var i

JavaScript strict warning: 
chrome://global/content/filepicker.js line 62: reference to undefined property 
o.displayDirectory.path

Comment 1

[Jarrod Gray]

Attachment: 4/5 fix...confused on undefined property

Comment 2

[Peter Trudelle]

->bryner/future

Comment 3

[Brian Ryner (not reading)]

setting to mozilla 1.0 -- would be nice to fix (and I will try to get this patch
checked in as a partial fix)

r=bryner on jarrod gray's patch

Comment 4

[Brian Ryner (not reading)]

ben, can you sr= this patch which fixes *most* of the strict warnings?

Comment 5

[Brian Ryner (not reading)]

Attachment: patch #2

Comment 6

[Brian Ryner (not reading)]

I took a slightly more aggressive cleanup approach :)

Here are the changes:

- get rid of all the strict warnings
- remove extraneous code copied from strres.js and update what's left to be the
same as in strres.js.
- change several dump()'s to debug()'s to reduce console spew

Please ignore the section of the patch with:
+          if (filePickerMode == nsIFilePicker.modeGetFolder)
+            textInput.value="";
I didn't mean to include it here.

jag, ben, pavlov, can you guys give me some combination of r= and sr=? :)

Comment 7

[Peter "jag" Annema]

It looks okay, but some nits:

+const nsILocalFile_CONTRACTID = "@mozilla.org/file/local;1";

that should be ALL_CAPS:
+const LOCAL_FILE_CONTRACTID = "@mozilla.org/file/local;1";

Related to this, perhaps you could turn the rest of the IIDs and CONTRACTIDs
into constants too? That would be the locale and stringbundle related ones.

There are still some dumps left which should be debugs (the ones you added with
the locale service stuff).

There's a shortcut for getting a service, e.g.:

localeService = Components.classes[LOCALE_SERVICE_CONTRACTID]
                          .getService(nsILocaleService);

Comment 8

[Brian Ryner (not reading)]

I left in the dump()'s which were for "real" error messages-- i.e. the ones I
would not expect to see.  Output like this would be very helpful for diagnosing
a regression in a nightly build.

I'm attaching a new patch with your other suggestions incorporated.

Comment 9

[Brian Ryner (not reading)]

Attachment: patch #3

Comment 10

[Peter "jag" Annema]

So you catch any exceptions, then return a null, for which we don't seem to be
checking. What would happen if we wouldn't catch it, or dump something useful
then throw it on (instead of returning a null)? Would filepicker not start? That
would be bad (no clear feedback to the user).

Not being able to get the stringbundle is rather severe, perhaps we could pop up
an error dialog in those cases?
However, if so desired (for fail-safe reasons?) we could fall back to some
default bundle (create an object which provides the GetStringFromName function
and returns something useful (in English) for those five strings).

Personally, I'd rather we not start the filepicker and give some clear error
feedback.

r=jag on the third patch, the above comments are for another bug/discussion.

Comment 11

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

There have been cases in the past where string bundles have failed to load, we 
handled them there by using hardcoded strings. (So unlikely a circumstance that 
English is better than nothing!) 

Note that that failure was in the profile manager, only on Mac (a result of some 
weird startup condition)... and this has not been experienced anywhere else in 
the chrome, there are numerous places I think where we just fail messily. 

a=[e-mail]

Comment 12

[Brian Ryner (not reading)]

checked in

Comment 13

[Henrik Gemal]

verified with build 20010126 on win2k

Comment 14

[Henrik Gemal]

sorry for the spam.... just changing the subject so that the bugs can be sorted 
correctly