Closed Bug 60632 Opened 24 years ago Closed 24 years ago

javascript strict warnings in importDialog.js

Categories

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: bugzilla, Assigned: hwaara)

Details

Attachments

(4 files)

JavaScript strict warning: chrome://messenger/content/importDialog.js line 204: redeclaration of var meterText JavaScript strict warning: chrome://messenger/content/importDialog.js line 344: redeclaration of var deck JavaScript strict warning: chrome://messenger/content/importDialog.js line 520: redeclaration of var result JavaScript strict warning: chrome://messenger/content/importDialog.js line 67: reference to undefined property window.arguments.importType
reassigning to chuang.
Assignee: putterman → chuang
QA Contact: esther → nbaca
The only thing I'm not perfectly sure about in the patch is: - if (window.arguments && window.arguments[0] && window.arguments.importTy pe) + if (window.arguments && window.arguments[0] && window.arguments[0].impor tType) Please let me know if this is incorrect. Everything seems to function as intended, but i may not have covered all possible uses.
Keywords: patch
Keywords: review
- if (window.arguments && window.arguments[0] && window.arguments.importType) + if (window.arguments && window.arguments[0] && + "importType" in window.arguments[0] && window.arguments[0].importType) I think you want to check for window.arguments.length >= 1 instead of tentatively looking at window.arguments[0] to see if it exists. Looks fine otherwise...
There seems to be much more precedence for |if (window.arguments[0])| in lxr than |if (window.arguments.length > 0)| Am I missing a deeper problem, or are you just suggesting this kind of change? If so, could I get a conditional r= based on this simple change?
Oh, sorry, the conditional r= was implied... Now, the question is, do you want to know if |arguments[0]| exists, or do you already know it exists but that it isn't |null|? If you want to know if the array called arguments has at least one member, then the cleaner (as in, won't give you an exception) way is to see if the length >= 1. If you already know it must have at least one member and you only want to know it isn't |null|, then what you have is fine.
Attached patch updated patchSplinter Review
cc'ing sspitzer for sr
tony, you around to review this?
I'm certainly no JS expert but I don't see anything that would change the functionality so it looks good to me.
it looks ok to me, but I'd need to test it first. delegating to chuang. candice, can you review / test this patch out?
window.arguments :-(
It looks good to me, but I'm not the module owner. Tony, can you check it?
Assignee: chuang → tonyr
candice, tony has already reviewed it. I was hoping you could test it and review it, too. timeless, can you elaborate?
I'll test it.
I only tested on importing ldif file. There's no JavaScript strict warning. I can't test on Mail or othe type address books.
thanks for testing it. sounds like we have a r=tonyr and r=chuang. patch looks good to me, sr=sspitzer
Checked in, completely forgot to fix the window.arguments nit, luckily mao has another patch pending for this file, in which he'll address that. Marking this fixed.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
window.arguments should be "arguments" in window, jag/mao understood, i'm sorry i didn't have time then/now to explain in more detail.
After running Tasks | Tools | Import Utility with the pref user_pref(javascript.options.strict, true); in a debug build, and importing address books, mail and settings, I got no JS strict errors (got some XBL stuff though.) Marking verified.
Status: RESOLVED → VERIFIED
When importing Outlook Express addressbook: JavaScript strict warning: chrome://messenger/content/importDialog.js line 307: assignment to undeclared variable pcnt
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
QA Contact: nbaca → fenella
I'll fix the last error so we can mark this bug FIXED.
r=mao
sr=jst
taking
Assignee: tonyr → hwaara
Status: REOPENED → NEW
alright, fix checked in. don't reopen this bug if you find warnings in importDialog.js in the future, open a new bug instead.
Status: NEW → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
Developer, please advise how to verify this bug.
I have no idea; ask gemal how he found it in the first place. ;)
You can check it with a debugging build. If you can't see any warning from the console window when doing the import, then it is fixed.
you have to set a pref to turn on strict warnings, which I think you can do in a debug or opt build. stephend knows for sure.
Yeah, I walked through this with Fenella... if JavaScript error: chrome://communicator/content/securityUI.js line 30: Components.classes['@mozill a.org/secure_browser_ui;1'] has no properties isn't part of the Import Utility (doesn't look like it), then we can mark this verified, I saw no other warnings. In prefs.js, add: user_pref("javascript.options.strict", true); run in console mode: netscp6.exe -console
Win32 (2001-04-02-06 trunk) This is fine.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: