javascript strict warnings in addressbook.js

RESOLVED FIXED in mozilla1.8alpha2

Status

MailNews Core
Address Book
--
trivial
RESOLVED FIXED
18 years ago
10 years ago

People

(Reporter: Jarrod Gray, Assigned: sgautherie)

Tracking

Trunk
mozilla1.8alpha2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 obsolete attachments)

(Reporter)

Description

18 years ago
JavaScript strict warning: 
chrome://messenger/content/addressbook/addressbook.js line 261: function 
AbDeleteDirectory does not always return a value

JavaScript strict warning: 
chrome://messenger/content/addressbook/addressbook.js line 279: redeclaration of 
var parentId

JavaScript strict warning: 
chrome://messenger/content/addressbook/addressbook.js line 306: function 
AbDeleteDirectory does not always return a value

JavaScript strict warning: 
chrome://messenger/content/addressbook/addressbook.js line 310: function 
AbDeleteDirectory does not always return a value

JavaScript strict warning: 
chrome://messenger/content/addressbook/addressbook.js line 76: assignment to 
undeclared variable menuitem

Comment 1

18 years ago
confirmed, ->me
Assignee: putterman → blakeross
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

18 years ago
Created attachment 18644 [details] [diff] [review]
(Av1) <addressbook.js>

Updated

17 years ago
Summary: strict warnings in addressbook.js → Lots of strict warnings in addressbook.js

Updated

17 years ago
Summary: Lots of strict warnings in addressbook.js → javascript strict warnings in addressbook.js

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: P3 → P5
Hardware: PC → All
Target Milestone: --- → Future

Comment 3

17 years ago
reassigning.
Assignee: blakeross → putterman
Status: ASSIGNED → NEW

Updated

17 years ago
Assignee: putterman → chuang

Comment 4

17 years ago
reassigning to chuang.

Updated

17 years ago
Keywords: patch

Comment 5

17 years ago
is this patch ready for sr?
When you fix this, can someone verify, thanks!

Comment 7

17 years ago
r=hwaara

just email one of the mailnews/addressbook super-reviewers now so they can sr=
and check it in.
Keywords: approval

Comment 8

17 years ago
What's happening here?
We have the patch, who can sr?
Seth can, but he prefers you e-mail sspitzer@netscape.com with this bug # and
cc: reviewers@mozilla.org.
QA Contact: esther → stephend

Comment 10

17 years ago
this now also:
Warning: reference to undefined property top.addressbook.setWebShellWindow
Source File: chrome://messenger/content/addressbook/addressbook.js
Line: 52
Keywords: approval, patch

Comment 11

17 years ago
reassigning to racham.
Assignee: chuang → racham

Comment 12

16 years ago
*** Bug 99504 has been marked as a duplicate of this bug. ***
mass re-assign.
Assignee: racham → sspitzer
(Assignee)

Comment 14

14 years ago
Comment on attachment 18644 [details] [diff] [review]
(Av1) <addressbook.js>

>@@ -73,7 +73,7 @@
>-	menuitem = top.document.getElementById(menuitemID);
>+	var menuitem = top.document.getElementById(menuitemID);

Fixed in r1.71 (Bug 108604).

>@@ -254,7 +254,7 @@
>     if (count == 0)
>-        return;
>+        return true;

Fixed, the other way round, in r1.71 (Bug 108604).

>@@ -273,14 +273,15 @@
>+		var parentId;
>                 if (parent == dirTree)
>-                    var parentId = "abdirectory://";
>+                    parentId = "abdirectory://";
>                 else	
>-                    var parentId = parent.getAttribute("id");
>+                    parentId = parent.getAttribute("id");

Fixed in r1.71 (Bug 108604).

>-                var parentDir = dirResource.QueryInterface(Components.interfaces.nsIAbDirectory);
>+                parentDir = dirResource.QueryInterface(Components.interfaces.nsIAbDirectory);

That was wrong.

>@@ -303,10 +304,11 @@
>         if(!window.confirm(confirmDeleteAddressbook))
>-            return;
>+            return true;
> 
>         top.addressbook.deleteAddressBooks(dirTree.database, parentArray, dirTree.selectedItems);
>     }
>+    return true;

Fixed, the other way round, in r1.71 (Bug 108604).
Attachment #18644 - Attachment description: proposed patch → (Av1) <addressbook.js>
Attachment #18644 - Attachment is obsolete: true
(Assignee)

Comment 15

14 years ago
(In reply to comment #10)
> this now also:
> Warning: reference to undefined property top.addressbook.setWebShellWindow
> Source File: chrome://messenger/content/addressbook/addressbook.js
> Line: 52

Fixed in r1.72 (Bug 73868).
Severity: normal → trivial
Priority: P5 → --
Target Milestone: Future → ---
(Assignee)

Comment 16

14 years ago
Created attachment 152140 [details] [diff] [review]
(Bv1-SM) <addressbook.js>
[Checked in: Comment 19]

Fixes
{{
Warning: assignment to undeclared variable gLDAPPrefsService
Source File: chrome://messenger/content/addressbook/addressbook.js
Line: 149
}}
and
improves 2 |.getService().QueryInterface(...)|
(Assignee)

Updated

14 years ago
Assignee: sspitzer → gautheri
Status: NEW → ASSIGNED
(Assignee)

Updated

14 years ago
Attachment #152140 - Flags: superreview?(mscott)
Attachment #152140 - Flags: review?(sspitzer)

Updated

14 years ago
Attachment #152140 - Flags: superreview?(mscott)
Attachment #152140 - Flags: superreview+
Attachment #152140 - Flags: review?(sspitzer)
Attachment #152140 - Flags: review+
(Assignee)

Comment 17

14 years ago
Created attachment 152178 [details] [diff] [review]
(Bv1-TB) <addressbook.js>
(Assignee)

Comment 18

14 years ago
Comment on attachment 152178 [details] [diff] [review]
(Bv1-TB) <addressbook.js>


I don't use FF+TB: Could you test/review/check in this patch ? Thanks.
Attachment #152178 - Flags: review?(mscott)
(Assignee)

Comment 19

14 years ago
Comment on attachment 152140 [details] [diff] [review]
(Bv1-SM) <addressbook.js>
[Checked in: Comment 19]


Check in: { 2004-07-02 02:05	neil%parkwaycc.co.uk	mozilla/ mailnews/
addrbook/ resources/ content/ addressbook.js	     1.108 }
Attachment #152140 - Attachment description: (Bv1-SM) <addressbook.js> → (Bv1-SM) <addressbook.js> [Checked in: Comment 19]
Attachment #152140 - Attachment is obsolete: true
Product: Browser → Seamonkey
(Assignee)

Comment 20

13 years ago
Comment on attachment 152178 [details] [diff] [review]
(Bv1-TB) <addressbook.js>

No super-review from <mscott@mozilla.org> since "2004-07-02" :-(

I don't use TB: Could you test/review/check in this patch ? Thanks.
Attachment #152178 - Flags: review?(mscott) → review?(bienvenu)
(Assignee)

Comment 21

13 years ago
Created attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

Bv1 diffed against current Trunk,
per
{{ (Mark Banner <bugzilla@standard8.demon.co.uk> 's email)
The patch works - but it currently bitrots (I've recently touched one of those
lines)
}}
Attachment #152178 - Attachment is obsolete: true
Attachment #189303 - Flags: superreview?(bienvenu)
Attachment #189303 - Flags: review?(bienvenu)
(Assignee)

Updated

13 years ago
Attachment #152178 - Flags: review?(bienvenu)

Comment 22

13 years ago
> Bv1 diffed against current Trunk,

What actual problem is this patch about to fix?
The changes are nice to have, but not actually necessary...
(Assignee)

Comment 23

13 years ago
(In reply to comment #22)
> > Bv1 diffed against current Trunk,
> 
> What actual problem is this patch about to fix?

The same as its SM counterpart: see comment 16.

> The changes are nice to have, but not actually necessary...

Well, I don't see what drawback it would have either...

Comment 24

13 years ago
Comment on attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

>-  var addrbookSession = Components.classes["@mozilla.org/addressbook/services/session;1"].getService().QueryInterface(Components.interfaces.nsIAddrBookSession);
>+  var addrbookSession =
>+        Components.classes["@mozilla.org/addressbook/services/session;1"]
>+                  .getService(Components.interfaces.nsIAddrBookSession);

Nice to have, but fixes nothing.

>   try {
>-      gLDAPPrefsService = Components.classes["@mozilla.org/ldapprefs-service;1"].getService();       
>-      gLDAPPrefsService = gLDAPPrefsService.QueryInterface( Components.interfaces.nsILDAPPrefsService);                  
>+    Components.classes["@mozilla.org/ldapprefs-service;1"]
>+              .getService(Components.interfaces.nsILDAPPrefsService);
>   } catch (ex) {dump ("ERROR: Cannot get the LDAP service\n" + ex + "\n");}

This is simply useless. What's the point of getting a service and throwing it
away in the same instant?

>-  var addrbookSession = Components.classes["@mozilla.org/addressbook/services/session;1"].getService().QueryInterface(Components.interfaces.nsIAddrBookSession);
>+  var addrbookSession =
>+        Components.classes["@mozilla.org/addressbook/services/session;1"]
>+                  .getService(Components.interfaces.nsIAddrBookSession);

Again, nice to have but no fix.

Either the nsILDAPPrefsService is used then it needs to stay somewhere where it
can be used. Or it isn't, then the entire try clause can be killed...
The check-in of this into SM really doesn't make much sense.
Attachment #189303 - Flags: review?(bienvenu) → review-
(Assignee)

Comment 25

13 years ago
(In reply to comment #24)
> (From update of attachment 189303 [details] [diff] [review] [edit])

> Nice to have, but fixes nothing.

Sure, yet:
Neil used to ask me to get rid of the |.getService(void).QueryInterface(zyx)|
syntax whenever I came across one: I simply keep doing just that.

> This is simply useless. What's the point of getting a service and throwing it
> away in the same instant?

I can only guess that it does what the associated comment says:
|//This migrates the LDAPServer Preferences from 4.x to mozilla format.|

> The check-in of this into SM really doesn't make much sense.

Scott would be the one to ask about that...
Target Milestone: --- → mozilla1.8alpha2

Comment 26

13 years ago
yes, the comment is right - if you want to remove that line, you need to find
another way of migrating the 4.x prefs (if you care about 4.x migration in
seamonkey)
(Assignee)

Comment 27

13 years ago
Comment on attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

Unsetting |mnyromyr: review-| after David's comment;
Could one of you re-review it ? Thanks.
Attachment #189303 - Flags: review- → review?(bienvenu)

Comment 28

13 years ago
Comment on attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

@David: this "fix" is already in SM, this is about the TB version.

Serge, the problem is this: the conversion of Netscape 4.x addressbooks is
currently only done in Netscape 6/7, because these are in a propriatory format
(see bug 35509 also). The code in this file here is part of the hook that NS
6/7 used to get this into the code. Since this conversion is part of neither
Mozilla/SM nor TB, we need to write that clean up from the bottom anyway -
if/when we try to fix it.
Your current patch produces useless code: On one hand, if the conversion code
/would/ be applied after your patch, it wouldn't work.	 OTOH, with this patch,
we  have cruft.
Netscape is no more, so we should remove it.
Attachment #189303 - Flags: review?(bienvenu) → review-
(Assignee)

Updated

13 years ago
Attachment #189303 - Flags: superreview?(bienvenu)
(Assignee)

Comment 29

13 years ago
Created attachment 189477 [details] [diff] [review]
(Bv2-TB) <addressbook.js> (& -SM sync.)
Attachment #189303 - Attachment is obsolete: true
Attachment #189477 - Flags: superreview?(bienvenu)
Attachment #189477 - Flags: review?(bienvenu)
(Assignee)

Comment 30

13 years ago
Comment on attachment 189477 [details] [diff] [review]
(Bv2-TB) <addressbook.js> (& -SM sync.)

Bv1a, with comment 28 suggestion(s).

Comment 31

13 years ago
Comment on attachment 189477 [details] [diff] [review]
(Bv2-TB) <addressbook.js> (& -SM sync.)

I don't want to remove this from Thunderbird. If you want to remove it from SM,
that's up the SM folks.
Attachment #189477 - Flags: review?(bienvenu) → review-
(In reply to comment #31)
> (From update of attachment 189477 [details] [diff] [review] [edit])
> I don't want to remove this from Thunderbird. If you want to remove it from SM,
> that's up the SM folks.
> 
I hadn't been watching this one. I'd like to think about it a bit more before SM
decides.

Comment 33

13 years ago
> I hadn't been watching this one. I'd like to think about it a bit more before SM
> decides.

It's just junk - it does nothing now and will still do nothing (apart from being
broken) if Netscape would return from the grave...

I've now taken an in-depth look at the migration code. The lines in question
that we are looking at call the constructor in nsLDAPPrefsService.js which sets
up a local list of directories and then calls migrate. The migrate function will
only run if a) it hasn't bee run before in that session or b) the preferance
prefs_migrated is set to false.

Unlike with local address books, we do actually successfully migrate ldap set ups.

Currently nsILDAPPrefsService is obtained in 4 different places (throughout the
seamonkey code - thunderbird is the same just in branched code). The places are:
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/addressbook.js#185
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/prefs/resources/content/pref-directory.js#20
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/resources/content/MsgComposeCommands.js#137
http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMessengerMigrator.cpp#736

addressbook.js and MsgComposeCommands.js only do a call to get the ldap prefs
service. MsgComposeCommands.js takes a copy of it but then doesn't actually use
it (and also the seamonkey version gets it wrong at the moment).

Having found that out, I then investigated what happens when we migrate
preferences from netscape 4.x. In most situations, the nsMessengerMigrator did
the migration before addressbook.js or MsgComposeCommands.js got to it. The only
case where this didn't occur was with preferences. Start up seamonkey with the
browser (and migrating 4.x), then go into prefs - the ldap prefs need to be
migrated there.

Just a thought - are there any cases where we can migrate a 4.x profile & switch
to it whilst running - I don't believe so.

I think that we can safely remove the nsILDAPPrefsService obtaining from
addressbook.js and MsgComposeCommands.js. We can then either

a) remove the call to migrate from the constructor of nsILDAPPrefsService and
call it explictly in pref-directory.js and nsMessengerMigrator.cpp or
b) leave the call where it is, slightly hidden.

I also think that we have some redundant calls and code.

There is some near duplicate code in:

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsLDAPPrefsService.js#76
 (in the constructor)
and
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/prefs/resources/content/pref-directory.js#141

which is sorting out the available functions. We could probably combine into one
function in nsILDAPPrefsService

That constructor code could then be moved into migrate - as that is the only bit
the uses it, and we won't then need to store availDirectories in
nsILDAPPrefsService.

Any Comments? I think we can probably get rid of some redundant code here and
tidy things up.

Comment 35

13 years ago
> The lines in question that we are looking at call the constructor in
> nsLDAPPrefsService.js which sets up a local list of directories and
> then calls migrate.

Hiding such stuff in a mere ctor call is truly evil!

> I think that we can safely remove the nsILDAPPrefsService obtaining from
> addressbook.js and MsgComposeCommands.js.

Good.

> We can then either
> 
> a) remove the call to migrate from the constructor of nsILDAPPrefsService and
> call it explictly in pref-directory.js and nsMessengerMigrator.cpp or
> b) leave the call where it is, slightly hidden.

I strongly vote for (a), be it at least in the light of code maintainability.

> I think we can probably get rid of some redundant code here and
> tidy things up.

Yeah, but that would better be a separate bug.
Mnyromyrs thoughts in comment 35 all sound reasonable to me.
(Assignee)

Updated

13 years ago
Assignee: gautheri → bugzilla
Status: ASSIGNED → NEW
I have just raised the following bugs for the spin-off issues that came up in
comment 34.

Bug 305434 LDAP Prefs Service needs revising.
Bug 305436 Consider whether we really need to try and migrate ldap prefs in
various locations. 
Comment on attachment 189477 [details] [diff] [review]
(Bv2-TB) <addressbook.js> (& -SM sync.)

Clearing now obsolete review request.
Attachment #189477 - Attachment is obsolete: true
Attachment #189477 - Flags: superreview?(bienvenu)
Comment on attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

Clearing obsolete flag and opening this patch back up. I'd like to put this in
to sync Thunderbird up with SeaMonkey, and then the other two bugs I mentioned
will handle the needed follow-on changes.
Attachment #189303 - Attachment is obsolete: false
Attachment #189303 - Flags: superreview?(bienvenu)
Attachment #189303 - Flags: review?(bienvenu)
Attachment #189303 - Flags: review-

Updated

13 years ago
Attachment #189303 - Flags: superreview?(bienvenu)
Attachment #189303 - Flags: superreview+
Attachment #189303 - Flags: review?(bienvenu)
Attachment #189303 - Flags: review+
(Assignee)

Comment 40

13 years ago
Comment on attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

'approval1.8b4=?':
Trivial warning fix, no risk.
I know this is not blocking the (TB) branch,
but, as its SM counterpart went in in MASv1.8a2, keeping the two in sync could
be good nonetheless.
Attachment #189303 - Flags: approval1.8b4?
(Assignee)

Updated

13 years ago
Assignee: bugzilla → gautheri
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED

Comment 41

13 years ago
Comment on attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

we don't need these on the branch. Trunk should be a fine place for these.
Attachment #189303 - Flags: approval1.8b4? → approval1.8b4-
Comment on attachment 189303 [details] [diff] [review]
(Bv1a-TB) <addressbook.js>
[Checked in: Comment 42]

I just checked this in on behalf of Serge.

/cvsroot/mozilla/mail/components/addrbook/content/addressbook.js,v  <-- 
addressbook.js
new revision: 1.21; previous revision: 1.20
Attachment #189303 - Attachment description: (Bv1a-TB) <addressbook.js> → (Bv1a-TB) <addressbook.js> [Checked-in]
(Assignee)

Updated

13 years ago
Attachment #189303 - Attachment description: (Bv1a-TB) <addressbook.js> [Checked-in] → (Bv1a-TB) <addressbook.js> [Checked in: Comment 42]
Attachment #189303 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.