1340947 - Remove legacy generator from fixIterator and use for-of in consumers.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Tooru Fujisawa [:arai]]

simpler plan than bug 1340935.

fixIterator returns an object with __iterator__ property, and its value is legacy generator.
we should remove it and use for-of or iterable protocol in consumers.

Comment 1

[Tooru Fujisawa [:arai]]

It would be more complicated issue.
so many add-ons use fixIterator with for-in.

So, to keep them working while avoid displaying warning message for legacy generator,
I'll implement legacy iterator protocol there, instead of legacy generator

Comment 2

[:aceman]

Thanks, converting fixIterator to a proper 'new' generator is a good plan and also fixing the internal callers to use for..of is fine.

If you can do something to still allow addons doing for..in that would be great. Until m-c core drops support for the legacy stuff. Maybe you could throw a warning about deprecated if you detect legacy usage, so addons get migrated faster.

Comment 3

[Tooru Fujisawa [:arai]]

try run (but the last part is for previous plan, not current plan in comment #1):
  https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=84553f9711f19f85a492a7649fff6d4243ac5e3c&selectedJob=78708605

Comment 4

[Jorg K (CEST = GMT+2)]

Tooru-san, I'm terribly sorry about the state of the C-C tree. You can see current perma-reds here:
https://mzl.la/2lrQTHC

Basically the red X2 is expected and the red X1, too, they are eight crashes, six from bug 1337865 and two from bug 1334874.

I don't know what caused the
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/cloudfile/test-cloudfile-backend-hightail.js
which contributed to the red Z. I haven't seen this one before.

Comment 5

[Tooru Fujisawa [:arai]]

now trying to bisect the issue.
unfortunately it's debug-build only and also doesn't reproduce locally...

Comment 6

[Tooru Fujisawa [:arai]]

it's even more unfortunate that the failure in test-cloudfile-backend-hightail.js is intermittent :P
I cannot believe that the change from "for-in" to "for-of" can cause intermittent, except the case that the testcase hardly depends on timing.

Comment 7

[Tooru Fujisawa [:arai]]

anyway, I should wait for bug 1342828, to continue testing.

Comment 8

[Martin Schröder [:mschroeder]]

All tests should run in a (try)build again, so do you plan to continue work on this issue?

Comment 9

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=23760faf3facc88864748c967017317b94c1bedf

Comment 10

[Tooru Fujisawa [:arai]]

okay, there's no new regression.
I'll post patches shortly

Comment 11

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1f170b05c552c46b4f332680b789d7ec362f8045&selectedJob=120918743

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Use for-of when iterating over fixIterator in calendar/.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use for-of when iterating over fixIterator in mail/.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use for-of when iterating over fixIterator in mail/components/.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Use for-of when iterating over fixIterator in mail/components/im/.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Use for-of when iterating over fixIterator in mail/test/mozmill/.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Use for-of when iterating over fixIterator in mailnews/.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Use for-of when iterating over fixIterator in mailnews/db/gloda/.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Use for-of when iterating over fixIterator in suite/.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Remove __iterator__ from fixIterator.

Comment 21

[Jorg K (CEST = GMT+2)]

I think you'll get faster service by shifting Magnus' reviews to me or Aceman.

BTW, is there more than changing 'in' to 'of'?

Comment 22

[Tooru Fujisawa [:arai]]

thank you for reviewing :)

(In reply to Jorg K (GMT+2) from comment #21)
> BTW, is there more than changing 'in' to 'of'?

no other changes, except for Part 9 that modifies fixIterator implementation.

Comment 23

[:aceman]

Comment on attachment 8893744 [details] [diff] [review]
Part 9: Remove __iterator__ from fixIterator.

Review of attachment 8893744 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks fine to me and should make our loops faster by not setting newStyle.__iterator__ . Unless that was still faster than for..of loops (inside the JS engine).

I'll give r+ after I check how bad we break addons with this...

::: mailnews/base/util/iteratorUtils.jsm
@@ +53,4 @@
>   *
>   *   @note This returns an object that can be used in 'for...of' loops.
>   *         Do not use 'for each...in'. 'for...in' may be used, but only as a
>   *         legacy feature.

You also need to loose the 'for...in' permission here.

Comment 24

[:aceman]

(In reply to :aceman from comment #23)
> ::: mailnews/base/util/iteratorUtils.jsm
> @@ +53,4 @@
> >   *
> >   *   @note This returns an object that can be used in 'for...of' loops.
> >   *         Do not use 'for each...in'. 'for...in' may be used, but only as a
> >   *         legacy feature.
> 
> You also need to loose the 'for...in' permission here.

I mean change it to "Do not use 'for each...in' nor 'for...in'." so that addon converters will see what is going on.

Comment 25

[Tooru Fujisawa [:arai]]

thank you for reviewing :)

(In reply to :aceman from comment #23)
> I'll give r+ after I check how bad we break addons with this...

unfortunately, there are a bunch of addons (~80) that uses fixIterator with for-in loop.
might be better contacting devs before landing this?


> You also need to loose the 'for...in' permission here.

okay, I'll fix the comment.

Comment 26

[:aceman]

Yes, you can post to the newsgroup, like we did for the STEEL removal (affects about 100 addons).

Comment 27

[Jorg K (CEST = GMT+2)]

Landing the reviewed parts so they don't rot.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b20aa3911223
Part 2: Use for-of when iterating over fixIterator in mail/. r=jorgk
https://hg.mozilla.org/comm-central/rev/c2fc5a624054
Part 3: Use for-of when iterating over fixIterator in mail/components/. r=jorgk
https://hg.mozilla.org/comm-central/rev/53a71c362409
Part 4: Use for-of when iterating over fixIterator in mail/components/im/. r=jorgk
https://hg.mozilla.org/comm-central/rev/e9fd1207afae
Part 5: Use for-of when iterating over fixIterator in mail/test/mozmill/. r=jorgk
https://hg.mozilla.org/comm-central/rev/92d39aabf5c8
Part 6: Use for-of when iterating over fixIterator in mailnews/. r=jorgk
https://hg.mozilla.org/comm-central/rev/ed0f3db96eb2
Part 7: Use for-of when iterating over fixIterator in mailnews/db/gloda/. r=jorgk DONTBUILD

Comment 29

[:aceman]

(In reply to :aceman from comment #26)
> Yes, you can post to the newsgroup, like we did for the STEEL removal
> (affects about 100 addons).

Arai, the newsgroup is mozilla.dev.apps.thunderbird.

Comment 30

[:aceman]

The affected addons include ExQuilla by Kent and Lightning by Fallen.
Kent, do your users follow Nightlies and can you fix the addon quickly?
Fallen, is the standalone Lightning on AMO still used and downloaded by users (on nightly)?

Comment 31

[Jorg K (CEST = GMT+2)]

Part 1 is fixing Calendar in Daily ;-)

Comment 32

[:aceman]

Sure, but I'm not sure when/if that propagates to the standalone version on AMO.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #30)
> The affected addons include ExQuilla by Kent and Lightning by Fallen.
And ThunderHTMLedit by Jorg K :-(

Comment 34

[Tooru Fujisawa [:arai]]

(In reply to :aceman from comment #29)
> (In reply to :aceman from comment #26)
> > Yes, you can post to the newsgroup, like we did for the STEEL removal
> > (affects about 100 addons).
> 
> Arai, the newsgroup is mozilla.dev.apps.thunderbird.

posted
https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/lXQH7YImlOA

Comment 35

[Ian Neal]

Comment on attachment 8893743 [details] [diff] [review]
Part 8: Use for-of when iterating over fixIterator in suite/.

Thanks, LGTM r=me

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7067b32d6404
Part 8: Use for-of when iterating over fixIterator in suite/. r=IanN

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/eeb9fb0aac79
Part 1: Use for-of when iterating over fixIterator in calendar/. r=jorgk

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8893735 [details] [diff] [review]
Part 1: Use for-of when iterating over fixIterator in calendar/.

I needed a patch to land and I'm comfortable to review a few s/ in / of / ;-)

Comment 39

[Kent James (:rkent)]

(In reply to :aceman from comment #30)
> The affected addons include ExQuilla by Kent and Lightning by Fallen.
> Kent, do your users follow Nightlies and can you fix the addon quickly?
> Fallen, is the standalone Lightning on AMO still used and downloaded by
> users (on nightly)?

Thanks for pointing that out. I really only maintain compatibility with esr versions in ExQuilla, so I have added a bug to fix that for the next major release.

Comment 40

[Tooru Fujisawa [:arai]]

fwiw, bug 1083476 added warning for legacy generator.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e4559de558c3
Part 9: Remove __iterator__ from fixIterator. r=aceman

Comment 42

[Jorg K (CEST = GMT+2)]

Landed with the comments adjusted as requested.

Sorry, I had to land this since the warning introduced in bug 1083476 causes a massive xpcshell test failure since in those tests warnings are turned into errors and make the test fail.

08:27:06     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "JavaScript 1.7's legacy generators are deprecated; consider using ES6 generators instead. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function* for details." {file: "resource:///modules/iteratorUtils.jsm" line: 71 column: 8 source: "        yield item;
08:27:06     INFO -  "}]"

As for add-on compatibility: Well, enough interfaces were removed during the last seven days that we will have a lot of failing add-ons in Daily anyway, so adding the iterators to the mix is not going to make it much worse.

Comment 43

[:aceman]

Comment on attachment 8893744 [details] [diff] [review]
Part 9: Remove __iterator__ from fixIterator.

Review of attachment 8893744 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 44

[:aceman]

(In reply to Kent James (:rkent) from comment #39)
> Thanks for pointing that out. I really only maintain compatibility with esr
> versions in ExQuilla, so I have added a bug to fix that for the next major
> release.

Converting to for..of would also work for you on ESR (both forms worked for a long time now).