[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a3) Gecko/20040817] (release) (W98SE) {{ Warning: redeclaration of var i Source File: chrome://messenger/content/mailWindowOverlay.js Line: 769, Column: 17 Source Code: for (var i = 0; i < pop3DownloadServersArray.length; i++) }} when opening MailNews.

There is another warning: {{ Warning: assignment to undeclared variable pop3Server Source File: chrome://messenger/content/mailWindowOverlay.js Line: 746 }}

2 warning fixes, plus 2 little "optimizations".

Comment on attachment 156703 [details] [diff] [review] (Av1) <mailwindowoverlay.js> (SeaMonkey part) I shouldn't review this because I don't have any deferred pop3 accounts, try bienvenu as it's his code. He might give you sr too as it's a small patch, if not you can ask me after you have his review. That said, in my opinion you should leave the indenting at 4 spaces, and the only "optimization" I think you should keep is the [index].AppendElement one.

Comment on attachment 156703 [details] [diff] [review] (Av1) <mailwindowoverlay.js> (SeaMonkey part) David: Neil wrote {{ ... in my opinion you should leave the indenting at 4 spaces, and the only "optimization" I think you should keep is the [index].AppendElement one. }} If you (likely) confirm this, I'll prepare a new patch for checkin...

Comment on attachment 156703 [details] [diff] [review] (Av1) <mailwindowoverlay.js> (SeaMonkey part) can you just remove this, instead of commenting out the already commented out line? +/* + else + { + //dump(currentServer.serverURI + "...skipping, already opened\n"); + } +*/ If you want, I can make that change when I check in...

Comment on attachment 156703 [details] [diff] [review] (Av1) <mailwindowoverlay.js> (SeaMonkey part) (In reply to comment #5) > (From update of attachment 156703 [details] [diff] [review]) > can you just remove this, instead of commenting out the already commented out > line? That was the idea ;-) > If you want, I can make that change when I check in... Your Edit+Checkin would be fine with me, (unless you prefer that I edit it myself, which I could do later today.)

'blocking1.8a5=?': No big deal, but we need an Edit+Checkin only, see comment 6.

New additional warnings: {{ Warning: redeclaration of var i Source File: chrome://messenger/content/mailWindowOverlay.js Line: 1969, Column: 13 Source Code: for (var i = 0; i < pop3DownloadServersArray.length; i++) Warning: reference to undefined property defaultServer.msgFolder Source File: chrome://messenger/content/mailWindowOverlay.js Line: 749 Warning: assignment to undeclared variable pop3Server Source File: chrome://messenger/content/mailWindowOverlay.js Line: 1919 }} |Warning: reference to undefined property defaultServer.msgFolder|: I don't know how to fix this one: helpwanted.

Comment on attachment 166208 [details] [diff] [review] (Av2) <mailWindowOverlay.js> (SeaMonkey part) [Checked in: Comment 13] Can you check it in too ?

David: Would you have time to r+sr this more than 1 month old patch ? Thanks.

+ (K) Regression: see bug 270343 comment 6.

Comment on attachment 166208 [details] [diff] [review] (Av2) <mailWindowOverlay.js> (SeaMonkey part) [Checked in: Comment 13] Check in: { 2005-01-04 16:51 neil%parkwaycc.co.uk mozilla/ mailnews/ base/ resources/ content/ mailWindowOverlay.js 1.212 9/13 Bug 256477 Fix JavaScript strict warnings p=gautheri@noos.fr r+sr=bienvenu } (nit: wrong bug number ;-<)

Same as Av2-SM. I don't use TB: Could you test/review/check in this patch ? Thanks.

(In reply to comment #8) > |Warning: reference to undefined property defaultServer.msgFolder|: Fixed by bug 270343 patch Cv1.

Comment on attachment 170411 [details] [diff] [review] (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] No review from <mscott@mozilla.org> since "2005-01-05" :-(

Comment on attachment 170411 [details] [diff] [review] (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] [Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE) No review from <bienvenu@nventure.com> aither since "2005-02-07" :-( Could you (super-)review/check in this patch ? Thanks.

(In reply to comment #17) > (From update of attachment 170411 [details] [diff] [review] [edit]) > [Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE) Brendan, here too, I'm surprised not to get the warning about |pop3Server|...

Is the question "why doesn't the undeclared global variable set at http://lxr.mozilla.org/mozilla/source/mail/base/content/mailWindowOverlay.js#1988 in CoalesceGetMsgsForPop3ServersByDestFolder cause a strict warning?" I don't know. It will warn only the first time per chrome window, after which the global var exists and reassignment to it won't cause any warning. Someone needs to debug. Serge, I know I've said this before: don't nominate warning fixes and cleanups at the last minute for releases, especially for security releases. Fix them in alpha milestones only. /be

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE) 1 fix, plus a little more. Could you (super-)review/check in this patch ? Thanks.

Comment on attachment 177026 [details] [diff] [review] (Cv1) <mailWindowOverlay.js> This one came from {{ 1.29 scott%scott-macgregor.org 2003-11-30 12:36 Add a try/catch around a method call in HandleMDNResponse to catch a weird JS error I have not tracked down yet. }}

(In reply to comment #19) > Is the question "why doesn't the undeclared global variable set at > http://lxr.mozilla.org/mozilla/source/mail/base/content/mailWindowOverlay.js#1988 > in CoalesceGetMsgsForPop3ServersByDestFolder cause a strict warning?" Yes. > I don't know. It will warn only the first time per chrome window, after which > the global var exists and reassignment to it won't cause any warning. Someone > needs to debug. I'm unskilled for this :-<

Cv1, unbitrotted. (Hoping to get a r/sr before next year: still wondering how others achieve to fix the same bugs I already have patch waiting :-/)

Serge, how does this relate to bug 260447? you set the dependency Could David do the r and mscott the sr?

(In reply to comment #24) > Serge, how does this relate to bug 260447? you set the dependency Looking back at this after so long: that bug seems related to, at least, comment 15 and bug 270343 comment 20; it's where (some of) the regression/warnings comes from. > Could David do the r and mscott the sr? Yes, any r/sr-er would have my thanks...

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] is this the right patch? I don't see how it could have to do with any warnings...

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] (In reply to comment #26) > is this the right patch? Yes: in the Cv1 patch, the fix was [ - } catch (ex) { return 0;} + } catch (ex) { return; } ] but in Cv1a only the little rewrite part stands: no more warning issue.

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] Checkin: { 2006-11-26 11:27 bugzilla%standard8.demon.co.uk } 'approval1.8.1.1=?': (Thunderbird only) 'approval-seamonkey1.1=?': Trivial U.I. code cleanup, no risk. (To stay in sync, before other potentialy upcoming patches.)

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] a=me for SeaMonkey 1.1 (mailnews/ part)

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] approved for 1.8 branch, a=mscott (via PM)

*** Bug 325098 has been marked as a duplicate of this bug. ***

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] missed 1.8.1.1, removing approval. Will try to get it next time.

Comment on attachment 170411 [details] [diff] [review] (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] Checkin: { 2006-12-04 09:09 bugzilla%standard8.demon.co.uk mozilla/mail/base/content/mailWindowOverlay.js 1.153 }

Comment on attachment 170411 [details] [diff] [review] (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] 'approval1.8.1.2=?': (Thunderbird only) Trivial U.I. code cleanup, no risk. (Sync' SM->TB.)

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] Checkin (/MailNews part): { 2006-12-22 07:13 neil%parkwaycc.co.uk mozilla/mailnews/base/resources/content/mailWindowOverlay.js 1.222.2.20 MOZILLA_1_8_BRANCH }

Comment on attachment 217503 [details] [diff] [review] (Cv1a) <mailWindowOverlay.js> [Checkins: Comment 28 & 35 & 38] Approved for 1.8 branch, a=jay for drivers.

Serge: Could you please get (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] re-reviewed, it's been a while since that patch was looked at. If mscott or bienvenu want this other patch, we can take it.

Already checked in: mozilla/mail/base/content/mailWindowOverlay.js 1.95.2.49 by bugzilla%standard8.demon.co.uk

(In reply to comment #37) > Serge: Could you please get (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment > 33] re-reviewed, it's been a while since that patch was looked at. If mscott { bienvenu@nventure.com 2006-11-12 04:49:53 PST Attachment #170411 [details] [diff]Flag review?(bienvenu@nventure.com) review+ mscott@mozilla.org 2006-11-25 17:51:05 PST Attachment #170411 [details] [diff]Flag superreview?(mscott@mozilla.org) superreview+ } Is 1(+) month that old ? > or bienvenu want this other patch, we can take it. { sgautherie.bz@free.fr 2006-12-04 15:50:32 PST Attachment #170411 [details] [diff]Flag approval1.8.1.2? } What is really missing is the approval, requested 9 days "only" after sr...

(In reply to comment #38) > Already checked in: > mozilla/mail/base/content/mailWindowOverlay.js 1.95.2.49 by That's attachment 217503 [details] [diff] [review], I was asking about attachment 170411 [details] [diff] [review] which is in the same file which confuses us as to whether one supersedes the other.

(In reply to comment #40) > {...} which is in the same file which confuses us as to whether one supersedes the other. B and C patches are not related to one another, apart from applying to the same file.

Comment on attachment 170411 [details] [diff] [review] (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] approved for 1.8 branch, a=dveditz for drivers

Comment on attachment 170411 [details] [diff] [review] (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] 'approval1.8.1.3=?': (Thunderbird only) It had 'dveditz: approval1.8.1.2+', but noone actually checked it in :-/

Scott or David, is the appoved patch too late for TB2? It also waits for checkin for two months.

Comment on attachment 170411 [details] [diff] [review] (Bv1-TB) <mailWindowOverlay.js> [Checkin: Comment 33] Doesn't meet the 1.8.1.4 criteria, especially since we're not shipping a tbird update (2.0 will be out around then). You can try again later maybe when there is going to be a tbird update.

Removing 'checkin needed' as it is unclear what should (not) be checked in. If the flag is still needed, please add a comment explaining *exactly* which of the patches should be checked in where, and make sure to have the appropriate reviews and/or approvals. Somewhat related, is there a particular reason this bug is not marked FIXED yet? Given the confusion (in discussion on IRC as well as, cf, comment #37, 39, 40 and 41), it might even be more appropriate to file a separate bug for followup issues.

Confusion started when Bv1-TB got approval1.8.1.2+, but was not checked in. Then, it got approval1.8.1.3? unanswered, then approval1.8.1.4-. Let's say I don't care any longer :-| R.Fixed.