Closed Bug 1123122 Opened 10 years ago Closed 7 years ago

Stop using legacy generators in comm-central

Categories

(MailNews Core :: Backend, defect)

All
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jcranmer, Unassigned)

References

Details

Unfortunately, I can't give a nice search for this. Legacy generators look like: function () { yield 5; } As opposed to ES6 generators: function* () { yield 5; } <https://dxr.mozilla.org/comm-central/search?q=yield+path%3A.js+-path%3Amozilla&case=true&redirect=true> matches both. The only sure-fire way to know which one is being used is to check the code. Although, if you're in test code, and you include asyncTestUtils, that's a good sign you're using legacy generators (add_task generally has better support, and that's in xpcshell core). Thankfully, legacy generators appear to be rather little used outside of test code and gloda.
So the tests have been converted in bug 1292569 by force :) Bug 1199296 should clear out all legacy generators as methods by rejecting them in core JS. Can there be any more occurrences? Maybe functions in functions and anonymous functions in expressions. When fixing bug 1292569 I actually reviewed all occurrences of 'yield' and added the asterisk to its parent function whatever it was. So I hope /mailnews and /mail is clean. I think there are still unreviewed occurrences in /chat and /suite. I haven't grepped calendar.
Depends on: 1292569
Hardware: x86_64 → All
So we do a survey in /chat, /suite and /calendar, fix any problems there and then close this bug? Or can we assume it's done since the system works after legacy generators were disabled by M-C in bug 1199296 and Aceman and Aleth fixed all (known) failures in bug 1292569?
Flags: needinfo?(philip.chee)
Flags: needinfo?(makemyday)
Flags: needinfo?(acelists)
(In reply to Jorg K (GMT+2, PTO during summer) from comment #2) > Or can we assume it's done since the system works after legacy generators > were disabled by M-C in bug 1199296 and Aceman and Aleth fixed all (known) > failures in bug 1292569? How do we know the "system" works? Have you looked at any Seamonkey tests or calendar runs? We need to wait until the relevant peers confirm that. From a quick glance, I am not sure about this occurrence: https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/modules/shim/Loader.jsm#226 https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/components/calGoogleCalendar.js#401 For SM, e.g. these look unconverted: https://dxr.mozilla.org/comm-central/source/suite/common/dataman/dataman.js#289 https://dxr.mozilla.org/comm-central/source/suite/common/tests/browser/browser_637020.js#47
Flags: needinfo?(acelists)
These are only occurences in the Google provider, not in calendar core code. For these occurrences, we should track this in a seperate bug if needed, as gdata requires to provide legacy support for postbox and friends, so maybe not all occurrences will go away.
Flags: needinfo?(makemyday)
(In reply to :aceman from comment #3) > How do we know the "system" works? You're right, without defining what "system" means, we don't know it's working. And for bug 1287067 I switched back from icaljs to ical C++, so I can't tell for sure that "calendar JS" is working when using Daily 51a1. > Have you looked at any Seamonkey tests or > calendar runs? Are there tests for SM? Calendar tests run as part of our test suite, right? For example there is one that fails every 1st of the month ;-) (Bug 1244818) > We need to wait until the relevant peers confirm that. Indeed, that why I NI'ed them. > From a quick glance, I am not sure about this occurrence: > https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/modules/ > shim/Loader.jsm#226 > https://dxr.mozilla.org/comm-central/source/calendar/providers/gdata/ > components/calGoogleCalendar.js#401 Indeed, thanks for the references. They look problematic. I don't use the Google provider, but I recall there was a test for it that due to many failures got (partly) disabled (bug 1266823).
(In reply to [:MakeMyDay] from comment #4) > These are only occurences in the Google provider, not in calendar core code. > For these occurrences, we should track this in a seperate bug if needed, as > gdata requires to provide legacy support for postbox and friends, so maybe > not all occurrences will go away. They will need to go away when m-c disables all legacy generators. So maybe the gdata code still works now as bug 1199296 disabled them for "methods", which may not apply to e.g. the anonymous function passed to task.spawn in the example. But who knows what class of legacy generators they disable next.
(In reply to Jorg K (GMT+2, PTO during summer) from comment #5) > > Have you looked at any Seamonkey tests or > > calendar runs? > Are there tests for SM? Calendar tests run as part of our test suite, right? > For example there is one that fails every 1st of the month ;-) (Bug 1244818) Seamonkey has their own build server and tests somewhere.
(In reply to :aceman from comment #6) > They will need to go away when m-c disables all legacy generators. So maybe > the gdata code still works now as bug 1199296 disabled them for "methods", > which may not apply to e.g. the anonymous function passed to task.spawn in > the example. But who knows what class of legacy generators they disable next. Especially the shim loader is to implement newer js functionality for older Gecko version (down to TB 8, iirc), so having it in there does not neccessarilly mean it has to go away. Maybe an addional try/catch wrapper is required, but I leave that for Fallen to decide which route to take for it. This is also why I suggested to track that in a seperate bug.
Sure, there is no problem with a new bug. Please file it and block this one so we know what areas are still unconverted.
We might just bite the bullet and remove gecko 8 compat, at least to a level where we can use new language features. If Postbox wants to continue support then I might consider building a local version that uses babel to replace the es6 features.
It's up to the maintainers of course, but personally I never understood why add-ons would do anything but version-to-version match. If you use software version X you use the add-on for version X.
Depends on: 1340935
triggered try run that will log the place of "yield" of legacy generator. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a79647927b4774a349d12e1d37c7266f0bbbf58f it will be printed like following in build/test logs: LEGACY_GENERATOR: resource:///modules/iteratorUtils.jsm:77:8
Depends on: 1340947
so far the try run above hits the following places: resource:///modules/iteratorUtils.jsm:77:8 resource://gdata-provider/modules/gdataSession.jsm:378:23 resource://gdata-provider/modules/gdataSession.jsm:381:16 resource://gdata-provider/modules/gdataSession.jsm:385:16 resource://gdata-provider/modules/gdataSession.jsm:390:38 resource://gdata-provider/modules/gdataSession.jsm:392:38 resource://gdata-provider/modules/gdataUtils.jsm:1003:20 resource://gdata-provider/modules/gdataUtils.jsm:1005:24 resource://gdata-provider/modules/gdataUtils.jsm:1057:20 resource://gdata-provider/modules/gdataUtils.jsm:1060:20 resource://gdata-provider/modules/gdataUtils.jsm:1077:28 resource://gdata-provider/modules/gdataUtils.jsm:1086:20 resource://gdata-provider/modules/gdataUtils.jsm:1091:16 resource://gdata-provider/modules/gdataUtils.jsm:1150:16 resource://gdata-provider/modules/gdataUtils.jsm:1152:16 resource://gdata-provider/modules/gdataUtils.jsm:1181:28 resource://gdata-provider/modules/gdataUtils.jsm:1183:20 resource://gdata-provider/modules/gdataUtils.jsm:1205:20 resource://gdata-provider/modules/gdataUtils.jsm:1301:38 iteratorUtils.jsm will be handled in bug 1340947.
Depends on: 1342734
all depending bugs are fixed. feel free to reopen if there's any remaining usage.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: needinfo?(philip.chee)
You need to log in before you can comment on or make changes to this bug.