Closed Bug 1652885 Opened 2 years ago Closed 2 years ago

Dynamic registration and unregistration of calendar providers

Categories

(Calendar :: Internal Components, defect, P1)

Tracking

(thunderbird_esr78+ fixed, thunderbird80 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird80 --- fixed

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Fix - v0 (obsolete) β€” β€” Splinter Review

Calendar Providers are currently xpcom components, coming from the era of legacy xul add-ons. We need a way to dynamically register these, so that a WebExtension can register a provider on enable and remove it on disable/uninstall.

When a provide is unregistered, the calendars for that provider need to turn into calDummyCalendars, which gives them the look like the provider is missing.

I'm uploading v0 of the patch so others can test, this is missing tests.

Assignee: nobody → philipp
Status: NEW → ASSIGNED

The patch in bug 1652888 might be necessary for this to fully work.

Depends on: 1652888
Attachment #9163648 - Attachment is obsolete: true

We will want this for 78.1.1

Severity: -- → S1
Flags: needinfo?(paul)

I reviewed this on Monday (27th), but I forgot to set "request changes" in phabricator when I did it. Just fixed that now so the status is clearer.

Flags: needinfo?(paul)

(In reply to Philipp Kewisch [:Fallen] [:πŸ“†][:🧩] from comment #5)

Try run with review items fixed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=edf2dfeffb05c23a31027297e141279a92b570da

Looks successful and with Paul's comment "With a successful try run and the comments addressed, this is ready to land." we can take this and bug 1652888 for a beta 80.0b1 rebuild.

Can you land this on c-c Sunday morning?

Flags: needinfo?(philipp)

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/7ed9c0ba9c40
Dynamic registration and unregistration of calendar providers. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Comment on attachment 9166239 [details]
Bug 1652885 - Dynamic registration and unregistration of calendar providers. r?pmorris!

Approved for beta - needed for gdata provider

Rob will be landing on c-c

Attachment #9166239 - Flags: approval-comm-beta+

Comment on attachment 9166239 [details]
Bug 1652885 - Dynamic registration and unregistration of calendar providers. r?pmorris!

[Triage Comment]
Per Wayne in Matrix.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/df4787c209e6
follow-up - Mark second argument of calICalendarManager.unregisterCalendarProvider as optional. rs=bustage-fix

Comment on attachment 9166239 [details]
Bug 1652885 - Dynamic registration and unregistration of calendar providers. r?pmorris!

[Approval Request Comment]
Developer impact if declined: Developers will need to copy/paste a lot of lines from the CalCalendarManager that replicate a core component of the calendar code, registering and unregistering calendars. If we update this code in the future, developers may still stick to the original code which could cause unknown side effects for those and other calendars.
User impact if declined: Less likelihood of provider extension developers upgrading to 78, which means less users can benefit from these extensions.
Testing completed (on c-c, etc.): Testing with the Provider for Google Calendar on Thunderbird 80.0b1 went well. No know reports for calendars being broken, though you probably have a better idea of it.
Risk to taking this patch (and alternatives if risky): This patch makes changes to calendar registration, if something goes wrong it could mean users don't have access to their calendars. The patch was written with backporting in mind so I deem the risk to be low.

Flags: needinfo?(philipp)
Attachment #9166239 - Flags: approval-comm-esr78?

Comment on attachment 9166239 [details]
Bug 1652885 - Dynamic registration and unregistration of calendar providers. r?pmorris!

[Triage Comment]
Approved for esr78

(by the time this gets released on esr, beta will have been live for several days - but will want to recheck BMO before releasing 78.1.1)

Flags: needinfo?(vseerror)
Attachment #9166239 - Flags: approval-comm-esr78? → approval-comm-esr78+
Flags: needinfo?(vseerror)
Blocks: 1664016
No longer blocks: 1664016
Regressions: 1664016

I don't think the dummy calendar implemented in this was a good idea. It leaves users with invalid albeit disabled calendars without warning. Users can enable those calendars and attempt to use them but it results in an error being thrown with no visual indication.

Priority: -- → P1
Regressions: 1791642
No longer regressions: 1791642
You need to log in before you can comment on or make changes to this bug.