Closed Bug 1597596 Opened 5 months ago Closed 3 months ago

Explore converting calendar to a system add-on

Categories

(Calendar :: General, task, P3)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pmorris, Assigned: pmorris)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

For bug 1493008, explore the feasibility of integrating Calendar into Thunderbird as a system add-on.

This WIP/experimental patch converts Calendar/Lightning into a system add-on. It puts it into the "features" directory instead of "extensions", along with a few other tweaks.

So far this appears to work just fine. All the Mochitests and xpcshell tests pass. (Except for some intermittent failures of browser_timezones.js that looked like an issue with the test when I investigated. It hangs waiting on a "popupshown" event that I assume can sometimes happen before the test starts waiting for it).

Philipp mentioned that system add-ons are technically the same as other add-ons, so where the docs mention that they must be restartless and multi-process compatible, that's apparently a Firefox policy matter rather than a technical requirement. (I assume it's related to ensuring system add-ons can be upgraded in the background without a restart.)

Assignee: nobody → paul
Status: NEW → ASSIGNED

This WIP patch experiments with how using JS DOM manipulation rather than XUL overlays might work. I started with the menu items because they are fiddly, with lots of small insertions here and there.

So far this seems like a feasible approach to me, at least comparable or a little better than pre-processor includes in XUL files. Compared to overlays, it actually makes it a little clearer what is in the DOM already and what is being added and where. The main drawback I see at this point is not having code highlighting for the XUL strings in your editor.

A file naming convention (like a "calendar-dom-" prefix) would help distinguish these JS files.

Thanks for exploring this Paul. One thing you could do is keep the xul in a separate file, then load it using fetch or similar, and then parse that to a xul fragment. This way you get syntax highlighting, but of course you have the overhead of a further file load.

This patch adds basic WebExtensions experiment scaffolding with an onStartup event handler to lightning. The idea is to move away from overlays and instead use the experiment to load Calendar UI, JS code, etc. (This is an atypical use of a webext experiment.)

Thanks Philipp for the tip about loading XUL files as a way to keep syntax highlighting. Might be worth considering, especially for some cases.

This WIP patch uses the webext experiment to set up the calendar menus, loading the calendar-dom-main-menus.js file via the experiment rather than an overlay file. Basically a proof of concept for a way to fully migrate calendar away from overlays.

The experiment's onStartup handler was firing too soon, when the chrome urls were not yet defined for the JS files, so I went with another approach to loading which uses browser.runtime.onStartup.... Maybe there's a better way, but this works well enough for current purposes.

Apparently the webext experiment works while calendar is still a "legacy" addon (with "legacy" property in manifest.json) that uses overlays and a chrome.manifest file. That's helpful because it means we could do a step by step migration of overlays into the experiment instead of having to move them all at once.

With help from https://searchfox.org/comm-central/source/common/src/ExtensionSupport.jsm
we can use the experiment's onStartup handler to listen for windows being loaded. (So no need for the webext part to handle load detection.)

Attachment #9110328 - Attachment is obsolete: true

Adds stylesheets to the main window via the experiment rather than overlay file. Unfortunately the results are not 100% the same. E.g. in the calendar tab, the tabs for day, week, month, etc. don't look right, and the colored bars at the top of each day in the month view don't extend all the way across. I tried all the type options (AGENT_SHEET, AUTHOR_SHEET, and USER_SHEET) but got the same results. So there's more to do to figure this out.

Adds JS and CSS to message windows via webext experiment rather than via overlay. Shows a set up for handling multiple window types.

These patches now cover the basic pieces needed to convert calendar into (1) a system addon that's (2) a webext experiment rather than a legacy addon, and that (3) no longer uses overlays.

This fixes the CSS issue by using the approach used in Overlays.jsm. The problem was that the styles we were loading were getting lower precedence than toolkit CSS files like radio.css.

This looks good, on the whole.

The big question now, as I see it, is do we want to keep the chrome.manifest file? If we do, we'll need to steal this part of ext-legacy.js to register it, but it does save us from a lot of work. It would mean we could keep using chrome:// URLs everywhere rather than changing every one of them to be a moz-extension:// URL. It would also do component registration for us, meaning all of the calendar components should carry on as they have been.

On the other hand, it's something we don't really want extensions to do in future. It ties us to a bunch of things from the past that may not be around much longer, and they're not the sort of things we want to fix in a rush if they are removed. Perhaps we should keep the file for now, but work towards getting rid of it.

On the other, other hand (I now have three hands, apparently) if we stopped building the calendar as an extension most of what I've just said would be moot. We'd still have chrome:// URLs, we could switch to static component registration (and eliminate the risk of dynamic component registration disappearing) and it would make switching L10n to Fluent possible. I don't know what the L10n story is for Firefox system add-ons, but I think it's something along the lines of "don't".

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

This looks good, on the whole.

The big question now, as I see it, is do we want to keep the chrome.manifest file? If we do, we'll need to steal this part of ext-legacy.js to register it, but it does save us from a lot of work. It would mean we could keep using chrome:// URLs everywhere rather than changing every one of them to be a moz-extension:// URL. It would also do component registration for us, meaning all of the calendar components should carry on as they have been.

We should only be using moz-extension:// on the WebExtension side of things. Everything else is likely better a resource:// uri, which should be fairly easy to register/unregister even without a chrome.manifest (nsIResProtocolHandler)

On the other hand, it's something we don't really want extensions to do in future. It ties us to a bunch of things from the past that may not be around much longer, and they're not the sort of things we want to fix in a rush if they are removed. Perhaps we should keep the file for now, but work towards getting rid of it.

+1 on working towards getting rid of it

...and it would make switching L10n to Fluent possible. I don't know what the L10n story is for Firefox system add-ons, but I think it's something along the lines of "don't".

My understanding is that we can register l10n directories for fluent for Lighting just as well, using L10nRegistry. I'm not sure l10n is really the blocker here.

We've been going back and forth on system add-on vs integrated a lot, to me it feels hard to commit to a specific decision. Each have their advantages, and no downside is great enough that it would make the decision obvious.

Looking over the alternatives, I think what we can say is, the system add-on route will be more work.
Since it would be special, that's not something we can really get away from: there will always be some cases (edge or not) where special attention is needed. Sometimes not all in our hands, e.g. localization differences, and packaging on linux distributions. We can also say with certainty, system add-on is a higher technical risk - but not huge one, since we could always convert over to direct integration later if/when needed. Still that would mean wasted effort, and possibly a temporary large hickup sometime in the future.

Another angle to having it be a system add-on is that going forwards, I think a large focus of the WX apis would probably be in data access. Calendar data is an important data source. If it's a system add-on, it would be slightly odd to have that add-on provide an API. But I suppose it's technically possible(?).

For the advantages we could gain from having it be a system add-on, the most focus has perhaps been on that it helps us develop APIs that also other add-ons could hook into. However, it seems most of the APIs that would be needed are not such that we would actually want/could provide for longer term stability. With this in mind... it might not be that good of a benefit.

One example that came up: the calendar parts of bug 1562313 are putting up resistance due to the timing of the static registration is different in an add-on than elsewhere.

I'm closing this bug now that we have decided to go with direct integration rather than making calendar a system add-on.

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 74
You need to log in before you can comment on or make changes to this bug.