Closed Bug 1113097 Opened 10 years ago Closed 10 years ago

Fix misuse of fixIterator

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 40.0

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(4 files, 3 obsolete files)

There are a lots of: for each (let element in fixIterator(xx)) This is mis-use actually, should be: for (let element in fixIterator()) Now we got lots of test failures. See https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=1a581f0a2e36 for example.
Attached patch Fix for calendar (obsolete) — Splinter Review
Attached patch Fix for mail and mailnews (obsolete) — Splinter Review
Attached patch Fix for suite (obsolete) — Splinter Review
Assignee: nobody → hiikezoe
There was a still misuse in mail/test/mozmill/cloudfile/test-cloudfile-add-account-dialog.js. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c953e2bd80d3
Attachment #8538468 - Attachment is obsolete: true
Attachment #8538958 - Flags: review?(philipp)
Attachment #8538469 - Attachment is obsolete: true
Attachment #8538959 - Flags: review?(mkmelin+mozilla)
Attached patch Fix for suiteSplinter Review
Attachment #8538470 - Attachment is obsolete: true
Attachment #8538961 - Flags: review?(neil)
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=c953e2bd80d3 On windows debug build all xpcshell tests and all mozmill tests failed but I think it's another issue.
Comment on attachment 8538958 [details] [diff] [review] Fix for calendar [checked in comment 31] Review of attachment 8538958 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch!
Attachment #8538958 - Flags: review?(philipp) → review+
Comment on attachment 8538959 [details] [diff] [review] Fix for mail and mailnews [checked in comment 31] Review of attachment 8538959 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/FilterListDialog.js @@ +763,5 @@ > // If it cannot, check all accounts to find a server > // that can have filters. > let allServers = MailServices.accounts.allServers; > + for (currentServer in fixIterator(allServers, > + Components.interfaces.nsIMsgIncomingServer)) let currentServer, while we're here
Attachment #8538959 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Summary: Fix mis use of fixIterator → Fix misuse of fixIterator
Target Milestone: --- → Thunderbird 37.0
So something must have changed in the JS core.
(In reply to :aceman from comment #33) > So something must have changed in the JS core. Probably Bug 783829
(From comment #32) > Missed one instance in /calendar/ > http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/ > calendar-dialog-utils.js?rev=fa839fc34b66&mark=318-321#318 > > Missed one instance in /mail. > http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane. > js?rev=7a80103c70c5&mark=2483-2484#2483
Flags: needinfo?(hiikezoe)
Flags: needinfo?(hiikezoe)
Attachment #8539839 - Flags: review?(philip.chee)
(In reply to Philip Chee from comment #35) > (From comment #32) > > Missed one instance in /calendar/ > > http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/ > > calendar-dialog-utils.js?rev=fa839fc34b66&mark=318-321#318 > > > > Missed one instance in /mail. > > http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane. > > js?rev=7a80103c70c5&mark=2483-2484#2483 Great! I did check in mailnews, mail and calendar again, and found another one. I hope there is no misuse now.
Comment on attachment 8539839 [details] [diff] [review] Overlooked part [checked-in Comment 41] > Great! I did check in mailnews, mail and calendar again, and found another > one. I hope there is no misuse now. Good catch!
Attachment #8539839 - Flags: review?(philip.chee) → review+
Actually for (let x in fixIterator()) is also wrong, or, rather, it will be when bug 1123117 is fixed.
Attachment #8538958 - Attachment description: Fix for calendar → Fix for calendar [checked in comment 31]
Attachment #8538959 - Attachment description: Fix for mail and mailnews → Fix for mail and mailnews [checked in comment 31]
I'd say this bug is done if the failures are fixed by the patches. Arguing whether for..of or for..in is better for fixIterator has its own bugs.
Keywords: checkin-needed
Whiteboard: [check in 2 remaining patches]
Version: unspecified → Trunk
Attachment #8539839 - Attachment description: Overlooked part → Overlooked part [checked-in Comment 41]
Whiteboard: [check in 2 remaining patches] → [check in 1remaining patch]
Whiteboard: [check in 1remaining patch] → [check in 1 remaining patch]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [check in 1 remaining patch]
Target Milestone: Thunderbird 37.0 → Thunderbird 40.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: