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)
SeaMonkey
MailNews: Address Book & Contacts
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: bugzilla, Assigned: hwaara)
Details
Attachments
(4 files)
2.74 KB,
patch
|
Details | Diff | Splinter Review | |
2.79 KB,
patch
|
Details | Diff | Splinter Review | |
2.80 KB,
patch
|
Details | Diff | Splinter Review | |
562 bytes,
patch
|
Details | Diff | Splinter Review |
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
Updated•24 years ago
|
QA Contact: esther → nbaca
Comment 2•24 years ago
|
||
Comment 3•24 years ago
|
||
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
Comment 4•24 years ago
|
||
Comment 5•24 years ago
|
||
- 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...
Comment 6•24 years ago
|
||
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?
Comment 7•24 years ago
|
||
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.
Comment 8•24 years ago
|
||
Comment 9•24 years ago
|
||
cc'ing sspitzer for sr
Comment 10•24 years ago
|
||
tony, you around to review this?
Comment 11•24 years ago
|
||
I'm certainly no JS expert but I don't see anything that would change the
functionality so it looks good to me.
Comment 12•24 years ago
|
||
it looks ok to me, but I'd need to test it first.
delegating to chuang. candice, can you review / test this patch out?
Comment 13•24 years ago
|
||
window.arguments
:-(
Comment 14•24 years ago
|
||
It looks good to me, but I'm not the module owner. Tony, can you check it?
Assignee: chuang → tonyr
Comment 15•24 years ago
|
||
candice, tony has already reviewed it.
I was hoping you could test it and review it, too.
timeless, can you elaborate?
Comment 16•24 years ago
|
||
I'll test it.
Comment 17•24 years ago
|
||
I only tested on importing ldif file. There's no JavaScript strict warning. I
can't test on Mail or othe type address books.
Comment 18•24 years ago
|
||
thanks for testing it.
sounds like we have a r=tonyr and r=chuang.
patch looks good to me, sr=sspitzer
Comment 19•24 years ago
|
||
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
Comment 20•24 years ago
|
||
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
Reporter | ||
Comment 22•24 years ago
|
||
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 → ---
Assignee | ||
Comment 23•24 years ago
|
||
I'll fix the last error so we can mark this bug FIXED.
Assignee | ||
Comment 24•24 years ago
|
||
Comment 25•24 years ago
|
||
r=mao
Comment 26•24 years ago
|
||
sr=jst
Assignee | ||
Comment 28•24 years ago
|
||
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 ago → 24 years ago
Resolution: --- → FIXED
Comment 29•24 years ago
|
||
Developer, please advise how to verify this bug.
Assignee | ||
Comment 30•24 years ago
|
||
I have no idea; ask gemal how he found it in the first place. ;)
Comment 31•24 years ago
|
||
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.
Comment 32•24 years ago
|
||
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
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•