Closed Bug 1626739 Opened 6 months ago Closed 6 months ago

Upgrade to ical.js 1.4.0

Categories

(Calendar :: ICAL.js Integration, task, P3)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 77.0

People

(Reporter: pmorris, Assigned: pmorris)

Details

Attachments

(3 files, 3 obsolete files)

Upgrade to new ical.js version 1.4.0:
https://github.com/mozilla-comm/ical.js/releases/tag/v1.4.0

Probably also use its new lenient mode. That could be done here or in a follow-up bug if needed.

This patch is only the changes from a diff that Philipp sent me by email. It just upgrades ical.js to the new version. I've made Philipp the author of the patch. Shouldn't need much review, but I'm following protocol.

I'll upload a part2 patch to enable the new lenient mode, which seems like a good idea based on the issues I ran into with the tests in bug 1546606 and those reported here: https://github.com/mozilla-comm/ical.js/issues/186.

Attachment #9137587 - Flags: review?(geoff)
Comment on attachment 9137587 [details] [diff] [review]
part1-upgrade-icaljs-0.patch

Good to see some action here.
Attachment #9137587 - Flags: review?(geoff) → review+
Status: NEW → ASSIGNED

Configures ical.js to use the new non-strict (lenient) mode.

Attachment #9137821 - Flags: review?(geoff)
Comment on attachment 9137821 [details] [diff] [review]
part2-use-new-lenient-mode-0.patch

I think this is the wrong place to do this. calBackendLoader.js or calICALJSComponents.js would be better. Take care not to load Ical.jsm unless we're going to use it.

Also, FWIW, `checkCalendarBinaryComponent` and the strings it uses are obsolete and can be removed. I'd also be keen to get rid of calICALJSComponents.js by moving the code into calBackendLoader.js.
Attachment #9137821 - Flags: review?(geoff) → review-

Thanks for the review. This just relocates the code setting icaljs to lenient mode.

I looked at merging those two files, but I'm still figuring out some of this XPCOM business. Might try to catch you on chat on that. I can also do another patch to remove that checkCalendarBinaryComponent function.

Attachment #9137821 - Attachment is obsolete: true
Attachment #9138980 - Flags: review?(geoff)

Removes the checkCalendarBinaryComponent function and related strings. I can open a follow-up bug to update or remove other "lightning" strings.

I looked some more at merging those two files and got further with understanding the XPCOM logic, but it was taking too long given various other priorities. I don't want to hold up the rest of this for that. So lets leave it for another day.

Attachment #9139194 - Flags: review?(geoff)

Revised to update the logging strings "Lightning's ical.js backend" -> "Thunderbird's ical.js backend", etc.

Attachment #9138980 - Attachment is obsolete: true
Attachment #9138980 - Flags: review?(geoff)
Attachment #9139274 - Flags: review?(geoff)
Attachment #9139194 - Flags: review?(geoff) → review+
Comment on attachment 9139274 [details] [diff] [review]
part2-use-new-lenient-mode-2.patch

This is fine but surely there's no need for the `var ICAL` to be at the top, just put it where you're using it.
Attachment #9139274 - Flags: review?(geoff) → review+

Thanks for the review. All fixed.

Attachment #9139274 - Attachment is obsolete: true
Attachment #9139849 - Flags: review+

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8df053dcb298f66bd77dc56b171290bb1c9af76e
The eslint errors do not occur for me locally (with mach or in my editor), so I think that's an intermittent try run thing.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/de2bf949a75c
Upgrade ical.js to version 1.4.0. r=darktrojan
https://hg.mozilla.org/comm-central/rev/0f0c5a1c67f0
Set ical.js to use the new non-strict (lenient) mode. r=darktrojan
https://hg.mozilla.org/comm-central/rev/9320f207dba3
Remove obsolete checkCalendarBinaryComponent function. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Flags: needinfo?(philipp)
Target Milestone: --- → 76
Flags: needinfo?(philipp)
Target Milestone: 76 → 77
You need to log in before you can comment on or make changes to this bug.