Fix misuse of fixIterator

RESOLVED FIXED in Thunderbird 40.0

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
Thunderbird 40.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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.
Posted patch Fix for calendar (obsolete) — Splinter Review
Posted patch Fix for mail and mailnews (obsolete) — Splinter Review
Posted 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)
Posted 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+
https://hg.mozilla.org/comm-central/rev/ddea495220c7
https://hg.mozilla.org/comm-central/rev/7a80103c70c5
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: 4 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.