A decision was recently made to directly integrate Calendar into Thunderbird rather than making it a system add-on. This bug tracks the specific step of integrating Calendar so that it is no longer an add-on. Related work will be tracked in separate bugs (see meta bug 1493008).

1 of 3 initial WIP patches that take the first steps of moving Calendar away from being an add-on. Basically just working with the build system to get various files (modules, components, plain JS, etc.) where they can be loaded without console errors (well, almost none).

This is still at rough WIP stage, but putting it up for feedback since I'm not that familiar with the build system. Also, there may be better locations for the files (so far I've just focused on getting them to load without errors). They currently arrive in places like:

dist/bin/calendar-js/
dist/bin/chrome/calendar/
dist/bin/chrome/lightning/
dist/bin/components/
dist/bin/modules/calendar/
dist/bin/modules/calendar/utils/

2 of 3 WIP patches. This one is just a mechanical find/replace to update the 'modules' paths.

3 of 3 WIP patches. This one just updates the paths for components and other non-module JS and JSON files, like the "calendar-js" files.

Still to do:

• a handful of xpcshell test failures to investigate
• remove overlays using pre-processor includes (no UI without this)
• styles/css
• localization
• etc.

Thanks for the feedback. I've gone ahead with integrating overlays etc. and plan to circle back around to address feedback on the first patches.

I now have all the XUL overlays and other items from jar.mn files integrated, with no errors or warnings in the console. There are some test failures when I ran them locally and some other things left to do to get this ready for review.

I've made many smaller commits to make the review process easier. We can squash them before landing if that makes more sense. Here's a try run that did not go well for some reason, and I'm out of time today to look further into it:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6511e21986785e291c301ddf62d9f19939025add

Your Try run is busted because you removed the Makefile containing the stage-package target that's referenced by testsuite-targets.mk. It's no longer needed so you can remove the reference. (At some point we'll have to fix a few things which rely on stage-package doing what it does but that shouldn't affect a Try run.)

Which reminds me, you're going to have to remove this bit in the tests which tries to install the Lightning XPI in the XPCShell process.

Thanks for the help. Here's the latest. I've removed that stage-package target from testsuite-targets.mk and the XPCShell function. I've now fixed all the failing XPCShell tests, and improved one of the failing Mochitests, although it's still failing intermittently (that's browser_imipBarEmail.js).

I'm still getting a busted try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6999ecdbcff3d22a95e9708f8f1e8d056d3f9cfb
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285253071&repo=try-comm-central&lineNumber=2759
Looks like l10n stuff for the calendar add-on which no longer exists.
I see this file (that references 'all-locale-repack.sh' in calendar code etc.) looks like it might need to go away or change somehow? (I need to look into the l10n stuff further, as I'm not familiar with it.)
https://searchfox.org/comm-central/source/taskcluster/ci/addon/kind.yml

I've also noticed that TB crashes and shuts down when on the 'preferences' page for a little bit of time, at least when the preferences tab was already open when you started the app. It didn't seem to happen when I checked by opening the tab (with it not open when the app started).

(In reply to Paul Morris [:pmorris] from comment #8)

I see this file (that references 'all-locale-repack.sh' in calendar code etc.) looks like it might need to go away or change somehow? (I need to look into the l10n stuff further, as I'm not familiar with it.)
https://searchfox.org/comm-central/source/taskcluster/ci/addon/kind.yml

You shouldn't need to worry about that at this point. Yes it will need to go away, but it's a completely separate piece that I added recently to take already built and packaged Lightning XPIs and combine them into one XPI with all locales. It doesn't run unless I ask it to.

I suspect you'd have more luck with a full build in your Try run. But first fix this:

package> Error: $SRCDIR/comm/mail/installer/package-manifest.in:310: Missing file(s): Thunderbird Daily.app/Contents/Resources/chrome/calendar/lightning
package> Error: $SRCDIR/comm/mail/installer/package-manifest.in:311: Missing file(s): Thunderbird Daily.app/Contents/Resources/chrome/calendar/lightning.manifest

(These errors are just ignored warnings on an artifact build.)

Also note that you can run mach package on a local build. I also recommend using a full build for that.

Thanks again. I've fixed those two lines, and switched to using full builds, which succeed locally. Running mach package locally succeeded (although I haven't really tested the results). Try run is still not working: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9e1b7bf73d8944d4115bea15523a988575933dbe

When I run the tests locally only two didn't pass (although some failures seem to be intermittent).

Unexpected Results
------------------
comm/calendar/test/browser/browser_timezones.js
  FAIL Test timed out -
  FAIL Found a Calendar:EventDialog after previous test timed out -
  FAIL Found a Calendar:EventDialog:Timezone after previous test timed out -
comm/calendar/test/browser/browser_todayPane.js
  FAIL Uncaught exception - at resource://testing-common/mozmill/utils.jsm:386 - TimeoutError: waitFor: Timeout exceeded for '() => mozmill.utils.getWindows("Calendar:EventDialog").length == 0'
Stack trace:
TimeoutError@resource://testing-common/mozmill/utils.jsm:386:13
waitFor@resource://testing-common/mozmill/utils.jsm:442:11
MozMillController.prototype.waitFor@resource://testing-common/mozmill/controller.jsm:791:9
invokeEventDialog@resource://testing-common/mozmill/CalendarUtils.jsm:337:14
async*createEvent@chrome://mochitests/content/browser/comm/calendar/test/browser/browser_todayPane.js:34:11
testTodayPane@chrome://mochitests/content/browser/comm/calendar/test/browser/browser_todayPane.js:59:9
Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1062:34
Tester_execTest@chrome://mochikit/content/browser-test.js:1097:11
nextTest/<@chrome://mochikit/content/browser-test.js:925:14
SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:808:67
  FAIL Found an unexpected Calendar:EventDialog at the end of test run -

The patches are ready for review, so I'm going to upload them (all 33, one at a time, sigh... should we be using phabricator instead?).

I haven't changed the location of the files (calendar-js, etc.) since we have the patches from bug 1562313 to coordinate with. I assume they will land after these.

Last one, 33 of 33.

Final comment on a previous patch, I see you removed the SeaMonkey compat. Do you have it covered elsewhere to remove any remaining SeaMonkey code, or was this the last of it?

I've left a few open where either I wasn't sure what the code does and I think Geoff may have written it, or where I have a comment that might substantially change the patch.

Thank you for this amazing effort, and keeping things in such small pieces for review. Great work!

On the mechanics of this, will this remove the addon/extension, then replace the entire Thunderbird+addon with the new program? How will this handle integration with Google Provider? What is involved for end users (testers)?

(In reply to Worcester12345 from comment #58)

On the mechanics of this, will this remove the addon/extension, then replace the entire Thunderbird+addon with the new program? How will this handle integration with Google Provider? What is involved for end users (testers)?

Just my 2 cents.

I believe it will remove the extension not the preferences.

The Google Provider appears to be dead and I switched to Google CalDAV since a provider extension was no longer available for beta and daily versions of Thunderbird. The Provider component has been removed from the Calendar product in Bugzilla.

Which brings up the question of will the Calendar product still remain or will it become a component of the Thunderbird product in Bugzilla.

FWIW I still see Lightning listed as an extension in Thunderbird Daily 74.0a1.

The provider has a new home on github.
https://github.com/kewisch/gdata-provider/wiki/FAQ

As an independent addon, it's continued inclusion in the thunderbird source was difficult to justify I assume, but you would have to ask the author really. I suggest you contact the Author if you have questions about its availability for use on Daily.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #46)

Would it be sensible to keep the /calendar/ part of the URL? I'm pretty sure
you can register this namespace in Thunderbird's jar files too.

I like putting the calendar modules at /modules/calendar/ like this patch does, (rather than /calendar/modules/). That seems simpler on the build system end, and also say if you want to search the code for all /modules/ URLs (for both TB and calendar modules). If it is /calendar/modules/ you have to remember that some modules are in one place and some are in another (and likely for other file types as well, components, etc.).

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #52)

Are you using the block pattern in other places? Otherwise maybe an IIFE
would be more common?

Yeah, we are now using this block pattern in all the custom elements files, and it seems simpler than an IIFE.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #57)

Final comment on a previous patch, I see you removed the SeaMonkey compat. Do you have it covered elsewhere to remove any remaining SeaMonkey code, or was this the last of it?

There's bug 1599212 for removing or relocating SeaMonkey code to /suite/. I want to reach out to SeaMonkey folks to coordinate on what to do with the code.

I've left a few open where either I wasn't sure what the code does and I think Geoff may have written it, or where I have a comment that might substantially change the patch.

Thank you for this amazing effort, and keeping things in such small pieces for review. Great work!

Thanks, and thanks for the quick review. I've made the suggested changes. I'll hold off on uploading the updated patches for now. (I see a couple still pending review from darktrojan.)

Also, there are a few mochitests that fail when I run them locally and I haven't gotten a successful try run yet, so more to do there.

And I assume we'll want to add some start-up code that disables Lightning if it is installed (and do any other migration that might be needed?) before landing these changes.

The questions posed in comments 58, 59, and 60 probably fit better on the meta bug 1493008. But, in short, I think if a user has Lightning installed, it should be removed (or possibly disabled) when they upgrade to a version of TB that has calendar integrated. (Disabling rather than removing would allow users to downgrade and re-enable without having to re-install Lightning, but on the other hand most won't downgrade and it is not hard to reinstall Lightning if needed. So I lean toward remove rather than disable.)

Calendar prefs and data should be maintained across the transition. I think that Google Provider will continue to work as it does now. The bugzilla calendar product or TB component question is definitely one to consider.

(In reply to Geoff Lankow (:darktrojan) from comment #63)

I'm assuming this is a straight find/replace/reformat job so my r+ is based on that.

Yep, a mechanical find/replace/reformat. I mentioned that on the first version of the patch, but then didn't mention it again with the newer version.

(In reply to Geoff Lankow (:darktrojan) from comment #64)

I'm pretty sure XPCOMUtils.defineLazyModuleGetter can be replaced with
ChromeUtils.defineModuleGetter where only three arguments are used or the
fourth is the same as the second.

I've made these changes, and thanks to your tip on IRC I updated the paths in allowed-dupes.mn and got a try run to build successfully.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3ee25fb5ca879ac1bbd858893fb7de323dc10817

Unfortunately it looks like most if not all of the calendar tests failed. It seems like the files aren't being packaged correctly. There's an XML parsing error which looks like a problem with a DTD file lookup, and then a JS file not found error. That's in the Marionette test results. So I'm looking at whether I need to do something more/differently in package-manifest.in, and try mach package to see if I can reproduce/debug this locally.

With some help from Geoff the packaging and try run/tests are now working. It took moving some files around, such as the calendar-js files (now in components) and the localization files. The patches are rebased on trunk, and the few that changed are ready for re-review. Also a few additional things have been cleaned up and fixed. Here's the current try run, now in process. New patches are on their way.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e14be7eb0a0af7ca216da732533641269a4ad31b

Various changes to make the packaging steps work, allowing try runs to run, etc.

New patch from Geoff for packaging l10n files.

A straight find/replace/reformat for modules paths.

New patch addressing Geoff's review.

Mostly straightforward changes to adapt to new file paths, but some changes involve l10n and the calendar timezones addon that are a little more involved. I have not confirmed that that addon still works after these changes. Could be done in a follow-up if needed.

Some small changes to avoid importing JS scripts twice.

New patch updating allowed-dupes.mn file.

On startup, if Lightning is installed, then uninstall it. I checked that this works by setting up a new profile with Thunderbird 68.4.1 with Lightning installed, then opening the same profile with a build from these patches, and checking that Lightning had been uninstalled. Also a test event and task were conserved across the migration.

Without this patch the following error appears in the console on first run with a fresh profile:

  Migrator error: ReferenceError: gDataMigrator is not defined calendar-management.js:423
    initHomeCalendar chrome://calendar/content/calendar-management.js:423
    loadCalendarManager chrome://calendar/content/calendar-management.js:122
    commonInitCalendar chrome://calendar/content/calendar-chrome-startup.js:28
    ltnOnLoad chrome://lightning/content/messenger-overlay-sidebar.js:386
    (Async: EventListener.handleEvent)
    <anonymous> chrome://lightning/content/messenger-overlay-sidebar.js:753

I doubt we need all of the calendar-migration-dialog.js code in messenger.xhtml scope, but sorting that out (possibly splitting up the code in that file), would likely make sense in a follow-up.

FINAL_TARGET_FILES['components'] -> FINAL_TARGET_FILES.components

As Geoff and I discussed on IRC, this new patch removes the code for the calendar-timezones addon since it is unused.

Updated so it applies on top of the previous patch that removes the calendar-timezones addon.

You've now got two copies of the same list and the comments are irrelevant. Also please file follow-up bugs to stop shipping all the skin files on all platforms, and to stop using the chrome/calendar/skin/common/icons/* files in favour of the messenger ones. I'd rather not do those things now and concentrate on finishing this bit.

Ah, of course, good point. Now fixed and I've made a note to file those follow-up bugs (too late to file them tonight).

(In reply to Geoff Lankow (:darktrojan) from comment #108)

Comment on attachment 9123384 [details] [diff] [review]
part35-On-startup-uninstall-Lightning-addon-if-installed-0.patch

I think this is alright. Lightning will be gone from Daily's extensions
directory, and in any other version it will be incompatible and not start.

In at least the "non-daily version" case, I think it would end up only disabled (for incompatible version reasons) and not uninstalled, right? That might lead to confusion where a user sees the calendar features in the UI but the add-on is disabled, maybe then they remove it but the UI stays the same, etc. If it's fully uninstalled then it's clearer what's going on.

Oh yeah, doing it is fine. I was just trying to work out what would happen if it was active when you did so, but then I decided that it never would be, under normal circumstances.

Okay, I see, that makes sense.

(In reply to Geoff Lankow (:darktrojan) from comment #117)

Normally we'd change a string's name when making a significant change to its
value. In this case I think you should instead remove both strings and the
code using them. unknownTimezonesError is not used by any live code, and
especially now that we're building calendar in,
missingCalendarTimezonesError is very very unlikely to be seen.

Okay, I did that. I changed the missingCalendarTimezonesError instance to just a console error.

The unknownTimezonesError was less straightforward. I've dropped the whole branch of the switch, to fall through to the default error message. I'm reluctant to lose the specificity of the timezones error, but don't see a way to do that cleanly without keeping the l10n string since the error is shown to the user. Perhaps that will be captured by logging the original exception with the cal.ERROR(ex) bit. This leaves STORAGE_UNKNOWN_TIMEZONES_ERROR unused, but I didn't delete it from the calIErrors.idl interface since it's part of that interface.

Also, I didn't see why you said that "unknownTimezonesError is not used by any live code". Can you say more? Not sure why that's not live code.

If this is good to go, then we are down to a couple of reviews from Philipp before landing this.

Nothing's used STORAGE_UNKNOWN_TIMEZONES_ERROR since 2011. That switch case will never happen.

(In reply to Geoff Lankow (:darktrojan) from comment #117)

Comment on attachment 9123433 [details] [diff] [review]
part3-1-Remove-code-for-the-calendar-timezones-addon-0.patch

Normally we'd change a string's name when making a significant change to its
value. In this case I think you should instead remove both strings and the
code using them. unknownTimezonesError is not used by any live code, and
especially now that we're building calendar in,
missingCalendarTimezonesError is very very unlikely to be seen.

If I'm not mistaken, I've seen that "unknownTimezonesError" a lot in the live, production versions of Thunderbird and Lighting; so I would not cry if it were removed or broken at the beginning of fixing this.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #122)

Does this work without a restart? This sounds like the thing we need to
verify in a few different scenarios with a test plan. Code looks fine though!

Good call on making a test plan for this. I discussed it a bit with mkmelin today, and then did some more testing. Here's a report.

1. Previously I'd tested this, and it worked as expected:

• set up a new profile on current release (68.4.1)
• install Lightning from ATN
• create a few events and tasks
• open profile with trunk build with calendar integrated

2. Today I tried that again but added disabling Lightning before opening the profile with the new build. That also worked fine. I got a console message saying:
   1580852521244 addons.xpi-utils WARN Add-on {e2fda1a4-762b-4020-b5ad-a41df1933103} is not compatible with application version.

3. Next I did the same except created the new profile on a trunk build without calendar integrated.
   (Building from this commit: c3b8a0f83f71 geoff comm No bug - Update in-tree WebExtensions documentation to match out-of-tree changes. rs=docs-only)
   I didn't install Lightning from ATN, but just clicked "enable" and "restart" to install it on initial startup.

This scenario did not work as expected. Lightning was not removed and these errors were in the console:

[calBackendLoader] Using Lightning's icaljs backend
console.log: WebExtensions: Loading unpacked extension from /.../obj-x86_64-pc-linux-gnu/dist/bin/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}
console.log: "Could not read manifest '/components/calDefaultACLManager.manifest'."
console.log: "Could not read manifest '/components/calImportExportModule.manifest'."
console.log: "Could not read manifest '/components/calMemoryCalendar.manifest'."
console.log: "Could not read manifest '/components/calStorageCalendar.manifest'."
console.log: "Could not read manifest '/components/calTimezoneService.manifest'."
console.log: "Could not read manifest '/components/calBackendLoader.manifest'."
console.log: "Could not read manifest '/components/calCompositeCalendar.manifest'."
console.log: "Could not read manifest '/components/calDavCalendar.manifest'."
console.log: "Could not read manifest '/components/calICSCalendar.manifest'."
console.log: "Could not read manifest '/components/calItemModule.manifest'."
console.log: "Could not read manifest '/components/calItipEmailTransport.manifest'."
console.log: "Could not read manifest '/components/calItipProtocolHandler.manifest'."
console.log: "Could not read manifest '/components/calSleepMonitor.manifest'."
console.log: "Could not read manifest '/components/lightningTextCalendarConverter.manifest'."
console.log: WebExtensions: Loading add-on preferences from  /.../obj-x86_64-pc-linux-gnu/dist/bin/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}
console.log: WebExtensions: Firing profile-after-change listeners for {e2fda1a4-762b-4020-b5ad-a41df1933103}
JavaScript error: resource:///modules/Overlays.jsm, line 506: NetworkError: A network error occurred.
console.error: "Error while attempting to uninstall Lightning addon:" (new Error("Cannot uninstall addon {e2fda1a4-762b-4020-b5ad-a41df1933103} from locked install location app-global", "resource://gre/modules/addons/XPIInstall.jsm", 4408))

4. Next, creating a new profile from trunk without lightning integrated (as in scenario 3 above), I tried to uninstall Lightning and re-install it from ATN, but the UI blocks you from uninstalling Lightning. You can only disable it. So I disabled it and then opened the profile on trunk build with calendar integrated. I got fewer errors but Lightning was still installed, still disabled, and unable to be removed:

[calBackendLoader] Using Lightning's icaljs backend
console.error: "Error while attempting to uninstall Lightning addon:" (new Error("Cannot uninstall addon {e2fda1a4-762b-4020-b5ad-a41df1933103} from locked install location app-global", "resource://gre/modules/addons/XPIInstall.jsm", 4408))

That's as far as I've gone, and I'm not sure what the solution might be.

I didn't have any such problems on trunk. I tried with Lightning enabled, disabled, and in the process of being disabled (that is, I didn't restart after disabling, so the next start happened in the new build). Both builds were from the current tip and run from objdir/dist/thunderbird after running mach package.

(In reply to Geoff Lankow (:darktrojan) from comment #125)

I didn't have any such problems on trunk. I tried with Lightning enabled, disabled, and in the process of being disabled (that is, I didn't restart after disabling, so the next start happened in the new build). Both builds were from the current tip and run from objdir/dist/thunderbird after running mach package.

Hm, that's good news. I did not do the mach package step, build from current tip, etc. I'll do that and report back when I get a chance.

I've now re-tested following Geoff's steps and three test cases, with a mach clobber then building from trunk, each time with mach package, and running from objdir/dist/thunderbird/thunderbird. I also got no errors, everything worked fine as expected. So this should be ready to land at this point, with further testing to follow.

Bits of the build configuration will need to be removed, or nightly will have a bad time. The repackaging job only runs on-demand, so it would've been fine, but it still needs to go.

This is ready to land. As I understand it the plan is for me to land it since there are so many patches.