Closed Bug 1608610 Opened 3 months ago Closed 2 months ago

Integrate Calendar into Thunderbird (actual integration step, non-meta)

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 74.0

People

(Reporter: pmorris, Assigned: pmorris)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(40 files, 41 obsolete files)

7.31 KB, patch
pmorris
: review+
Fallen
: review+
Details | Diff | Splinter Review
145.94 KB, patch
Details | Diff | Splinter Review
12.01 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
7.48 KB, patch
Details | Diff | Splinter Review
6.98 KB, patch
Details | Diff | Splinter Review
9.82 KB, patch
Details | Diff | Splinter Review
14.11 KB, patch
Details | Diff | Splinter Review
1.30 KB, patch
Details | Diff | Splinter Review
11.10 KB, patch
Details | Diff | Splinter Review
1.03 KB, patch
Details | Diff | Splinter Review
15.02 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
21.82 KB, patch
Details | Diff | Splinter Review
8.46 KB, patch
Details | Diff | Splinter Review
95.87 KB, patch
Details | Diff | Splinter Review
3.42 KB, patch
Details | Diff | Splinter Review
74.36 KB, patch
Details | Diff | Splinter Review
50.47 KB, patch
Details | Diff | Splinter Review
11.21 KB, patch
Details | Diff | Splinter Review
8.23 KB, patch
Details | Diff | Splinter Review
15.13 KB, patch
Details | Diff | Splinter Review
7.94 KB, patch
Details | Diff | Splinter Review
13.30 KB, patch
Details | Diff | Splinter Review
10.13 KB, patch
Details | Diff | Splinter Review
28.66 KB, patch
Details | Diff | Splinter Review
8.38 KB, patch
Details | Diff | Splinter Review
6.92 KB, patch
Details | Diff | Splinter Review
6.60 KB, patch
Details | Diff | Splinter Review
13.22 KB, patch
Details | Diff | Splinter Review
37.66 KB, patch
Details | Diff | Splinter Review
5.38 KB, patch
Details | Diff | Splinter Review
4.89 KB, patch
Details | Diff | Splinter Review
2.12 KB, patch
Details | Diff | Splinter Review
1.47 KB, patch
Details | Diff | Splinter Review
2.06 KB, patch
Fallen
: review+
darktrojan
: review+
Details | Diff | Splinter Review
1.80 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
14.99 KB, patch
pmorris
: review+
Details | Diff | Splinter Review
13.45 KB, patch
pmorris
: review+
Details | Diff | Splinter Review
9.13 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
5.29 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
7.07 KB, patch
rjl
: review+
Details | Diff | Splinter Review

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/

Attachment #9120229 - Flags: feedback?(philipp)
Attachment #9120229 - Flags: feedback?(mkmelin+mozilla)
Attachment #9120229 - Flags: feedback?(geoff)

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.
Status: NEW → ASSIGNED
Comment on attachment 9120229 [details] [diff] [review]
part1-wip-first-steps-calendar-integration-0.patch

Review of attachment 9120229 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe we can avoid the  dist/bin/calendar-js/  dist/bin/calendar/ distinction?

Modules I think should end up with all the others in dist/bin/modules

Then, I'm not sure if there should be a separate lightning and calendar folder. What's the distinction nowadays?
Attachment #9120229 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9120229 [details] [diff] [review]
part1-wip-first-steps-calendar-integration-0.patch

I'd put the calendar-js files in the components folder, since they are components. (Never did like the calendar-js folder.)

Not sure about leaving timezones/zones.json where it is. Perhaps it should be in modules/calendar.
Attachment #9120229 - Flags: feedback?(geoff) → feedback+

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.

Attachment #9120229 - Attachment is obsolete: true
Attachment #9120229 - Flags: feedback?(philipp)
Attachment #9121613 - Flags: review?(geoff)
Attachment #9120230 - Attachment is obsolete: true
Attachment #9121614 - Flags: review?(geoff)
Attachment #9120232 - Attachment is obsolete: true
Attachment #9121615 - Flags: review?(geoff)
Attachment #9121620 - Flags: review?(geoff)
Attachment #9121623 - Flags: review?(geoff)
Attachment #9121624 - Flags: review?(geoff)
Attachment #9121637 - Flags: review?(geoff)
Attachment #9121639 - Flags: review?(geoff)
Attachment #9121640 - Flags: review?(geoff)
Attachment #9121641 - Flags: review?(geoff)
Attachment #9121642 - Flags: review?(geoff)
Attachment #9121644 - Flags: review?(geoff)
Attachment #9121645 - Flags: review?(geoff)
Attachment #9121646 - Flags: review?(geoff)
Attachment #9121647 - Flags: review?(geoff)

Last one, 33 of 33.

Attachment #9121650 - Flags: review?(geoff)
Comment on attachment 9121613 [details] [diff] [review]
part1-1608610-Change-build-system-to-integrate-calendar-0.patch

Review of attachment 9121613 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the calendar changes, the mail changes look ok to me too but technically I'm not a peer there.

::: mail/installer/package-manifest.in
@@ +343,1 @@
>  [calendar]

You probably want to move the [calendar] heading above these files.
Attachment #9121613 - Flags: review+
Comment on attachment 9121614 [details] [diff] [review]
part2-Update-modules-paths-0.patch

Review of attachment 9121614 [details] [diff] [review]:
-----------------------------------------------------------------

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.
Comment on attachment 9121616 [details] [diff] [review]
part4-Integrate-messenger-overlay-accountCentral-xhtml-0.patch

Review of attachment 9121616 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/content/msgAccountCentral.xhtml
@@ +185,5 @@
>          </vbox>
>        </hbox>
>  #endif
>  
> +      <spacer id="lightning-newCalendar-separator" flex="1"/>

Are you using `lightning-` in other patches as well? Otherwise maybe change this to `calendar-`
Attachment #9121616 - Flags: review?(geoff) → review+
Comment on attachment 9121613 [details] [diff] [review]
part1-1608610-Change-build-system-to-integrate-calendar-0.patch

Review of attachment 9121613 [details] [diff] [review]:
-----------------------------------------------------------------

Magnus has granted me permission to review the mail parts for patches in this bug.

::: mail/installer/package-manifest.in
@@ +343,1 @@
>  [calendar]

You probably want to move the [calendar] heading above these files.
Attachment #9121613 - Flags: review?(geoff)
Attachment #9121618 - Flags: review?(geoff) → review+
Comment on attachment 9121620 [details] [diff] [review]
part6-Integrate-lightning-item-panel-xhtml-0.patch

Review of attachment 9121620 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/messenger.xhtml
@@ +67,5 @@
> +%calendarDTD;
> +<!ENTITY % eventDialogDTD SYSTEM "chrome://calendar/locale/calendar-event-dialog.dtd">
> +%eventDialogDTD;
> +<!ENTITY % toolbarDTD SYSTEM "chrome://lightning/locale/lightning-toolbar.dtd">
> +%toolbarDTD;

Do you want to add a comment to the .inc file as to which dtd files are required? This might make it easier to find out what can be removed if an inc is removed.
Attachment #9121620 - Flags: review?(geoff) → review+
Comment on attachment 9121621 [details] [diff] [review]
part7-Integrate-messenger-overlay-preferences-xhtml-0.patch

Review of attachment 9121621 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/aboutPreferences.xhtml
@@ +67,5 @@
> +%alarmsDTD;
> +<!ENTITY % categoriesDTD SYSTEM "chrome://calendar/locale/preferences/categories.dtd">
> +%categoriesDTD;
> +<!ENTITY % viewsDTD SYSTEM "chrome://calendar/locale/preferences/views.dtd">
> +%viewsDTD;

Same comment about dtds as in the last review.
Attachment #9121621 - Flags: review?(geoff) → review+
Comment on attachment 9121623 [details] [diff] [review]
part9-Integrate-imip-bar-overlay-inc-xhtml-0.patch

Review of attachment 9121623 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/messageWindow.xhtml
@@ +51,5 @@
>  %quickFilterBarDTD;
>  <!ENTITY % msgViewPickerDTD SYSTEM "chrome://messenger/locale/msgViewPickerOverlay.dtd" >
>  %msgViewPickerDTD;
> +<!ENTITY % lightningDTD SYSTEM "chrome://lightning/locale/lightning.dtd">
> +%lightningDTD;

Same comment about dtds as in the last two, I'll stop mentioning this for future reviews.
Attachment #9121623 - Flags: review?(geoff) → review+
Comment on attachment 9121624 [details] [diff] [review]
part10-Fix-msgHeaderView-element-is-null-error-0.patch

Review of attachment 9121624 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/lightning/content/imip-bar.js
@@ +545,1 @@
>  }

Are you using the block pattern in other places? Otherwise maybe an IIFE would be more common?
Attachment #9121624 - Flags: review?(geoff) → review+
Attachment #9121625 - Flags: review?(geoff) → review+
Attachment #9121626 - Flags: review?(geoff) → review+
Attachment #9121627 - Flags: review?(geoff) → review+
Attachment #9121628 - Flags: review?(geoff) → review+
Attachment #9121629 - Flags: review?(geoff) → review+
Attachment #9121630 - Flags: review?(geoff) → review+
Attachment #9121631 - Flags: review?(geoff) → review+
Attachment #9121632 - Flags: review?(geoff) → review+
Attachment #9121634 - Flags: review?(geoff) → review+
Attachment #9121637 - Flags: review?(geoff) → review+
Attachment #9121638 - Flags: review?(geoff) → review+
Comment on attachment 9121639 [details] [diff] [review]
part22-Inline-calendar-file-new-menu-items-0.patch

Review of attachment 9121639 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mainNavigationToolbox.inc.xhtml
@@ +49,5 @@
>                <menuitem id="menu_newNewMsgCmd" label="&newNewMsgCmd.label;"
>                          accesskey="&newNewMsgCmd.accesskey;"
>                          key="key_newMessage2"
>                          command="cmd_newMessage"/>
> +              <menuitem id="ltnNewEvent"

Can we rename these to either calendar-new-event or at least menu_calendar_newEvent?

@@ +54,5 @@
> +                        label="&lightning.menupopup.new.event.label;"
> +                        accesskey="&lightning.menupopup.new.event.accesskey;"
> +                        key="calendar-new-event-key"
> +                        command="calendar_new_event_command"/>
> +              <menuitem id="ltnNewTask"

calendar-new-task or menu_calendar_newTask ?

@@ +59,5 @@
> +                        label="&lightning.menupopup.new.task.label;"
> +                        accesskey="&lightning.menupopup.new.task.accesskey;"
> +                        key="calendar-new-todo-key"
> +                        command="calendar_new_todo_command"/>
> +              <menuseparator id="afterltnNewTask"

would also recommend to rename this separator.

@@ +90,5 @@
>                <menuitem id="newAccountMenuItem"
>                          label="&newOtherAccountsCmd.label;"
>                          accesskey="&newOtherAccountsCmd.accesskey;"
>                          oncommand="MsgAccountWizard();"/>
> +              <menuitem id="ltnNewCalendar" label="&lightning.menupopup.new.calendar.label;"

calendar-new-menuitem or newCalendarMenuItem ?
Attachment #9121639 - Flags: review?(geoff) → review+
Comment on attachment 9121640 [details] [diff] [review]
part23-Inline-calendar-edit-and-go-menu-items-0.patch

Review of attachment 9121640 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mainNavigationToolbox.inc.xhtml
@@ +344,5 @@
>                    oncommand="ToggleFavoriteFolderFlag();"/>
>          <menuitem id="menu_properties" label="&folderPropsCmd2.label;"
>                    accesskey="&folderPropsCmd.accesskey;"
>                    command="cmd_properties"/>
> +        <menuitem id="ltnCalendarProperties"

I'd recommend to adjust the name here as well, calendar-properties-menuitem or menu_calendarProperties

@@ +685,5 @@
>                <menuitem id="menu_prevFlaggedMsg"
>                          label="&prevStarredMsgCmd.label;"
>                          accesskey="&prevStarredMsgCmd.accesskey;"
>                          command="cmd_previousFlaggedMsg"/>
> +              <menuseparator id="ltnGoPreviousSeparator"/>

I'd recommend renaming this separator to be similar to other separators in this file.

@@ +711,5 @@
>                      accesskey="&goBackCmd.accesskey;" command="cmd_goBack"
>                      key="key_goBack"
>                      class="menuitem-iconic"/>
>            <menuseparator id="goNextSeparator"/>
> +          <menuitem id="ltnGoToToday"

calendar-today-menuitem or menu_calendarGotoToday ?
Attachment #9121640 - Flags: review?(geoff) → review+
Comment on attachment 9121641 [details] [diff] [review]
part24-Pre-process-calendar-view-menu-items-0.patch

Review of attachment 9121641 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/mainNavigationToolbox.inc.xhtml
@@ +369,5 @@
>      <menu id="menu_View"
>            label="&viewMenu.label;"
>            accesskey="&viewMenu.accesskey;">
>        <menupopup id="menu_View_Popup" onpopupshowing="view_init();">
> +        <!-- onToolbarsPopupShowingForTabType is calendar code -->

Do we want to rename this function so it is clear from the function name?
Attachment #9121641 - Flags: review?(geoff) → review+
Attachment #9121642 - Flags: review?(geoff) → review+
Attachment #9121643 - Flags: review?(geoff) → review+
Attachment #9121644 - Flags: review?(geoff) → review+
Comment on attachment 9121645 [details] [diff] [review]
part28-Inline-calendar-view-and-go-appmenu-items-0.patch

Review of attachment 9121645 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/customizableui/content/panelUI.inc.xhtml
@@ +660,5 @@
>                         class="subviewbutton subviewbutton-nav"
>                         label="&folderView.label;"
>                         closemenu="none"
>                         oncommand="PanelUI.showSubView('appMenu-foldersView', this)"/>
> +        <toolbarseparator id="appmenu_ltnViewMenuSeparator"/>

This and following items might benefit from renaming so we use calendar instead of ltn, like further below.
Attachment #9121645 - Flags: review?(geoff) → review+
Attachment #9121646 - Flags: review?(geoff) → review+
Attachment #9121647 - Flags: review?(geoff) → review+
Attachment #9121648 - Flags: review?(geoff) → review+
Attachment #9121649 - Flags: review?(geoff) → review+
Attachment #9121650 - Flags: review?(geoff) → review+

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!

Attachment #9121622 - Flags: review?(geoff) → review+

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.

Comment on attachment 9121614 [details] [diff] [review]
part2-Update-modules-paths-0.patch

I'm assuming this is a straight find/replace/reformat job so my r+ is based on that.
Attachment #9121614 - Flags: review?(geoff) → review+
Comment on attachment 9121614 [details] [diff] [review]
part2-Update-modules-paths-0.patch

Oh wait, I knew there was something else.

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 wouldn't be surprised to see the XPCOMUtils version disappear in the future so we might as well start preparing while we changing the lines anyway.
Attachment #9121615 - Flags: review?(geoff) → review+

(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.

Attachment #9121613 - Attachment is obsolete: true
Attachment #9123346 - Flags: review?(philipp)
Attachment #9123346 - Flags: review?(geoff)

New patch from Geoff for packaging l10n files.

Attachment #9123347 - Flags: review?(philipp)
Attachment #9123347 - Flags: review?(paul)

A straight find/replace/reformat for modules paths.

Attachment #9121614 - Attachment is obsolete: true

New patch addressing Geoff's review.

Attachment #9123349 - Flags: review?(philipp)
Attachment #9123349 - Flags: review?(geoff)

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.

Attachment #9121615 - Attachment is obsolete: true
Attachment #9123350 - Flags: review?(philipp)
Attachment #9123350 - Flags: review?(geoff)
Attachment #9121620 - Attachment is obsolete: true
Attachment #9121623 - Attachment is obsolete: true

Some small changes to avoid importing JS scripts twice.

Attachment #9121625 - Attachment is obsolete: true
Attachment #9123359 - Flags: review?(geoff)
Attachment #9121639 - Attachment is obsolete: true
Attachment #9121641 - Attachment is obsolete: true

New patch updating allowed-dupes.mn file.

Attachment #9123383 - Flags: review?(philipp)
Attachment #9123383 - Flags: review?(geoff)

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.

Attachment #9123384 - Flags: review?(philipp)
Attachment #9123384 - Flags: review?(geoff)

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.

Attachment #9123385 - Flags: review?(philipp)
Attachment #9123385 - Flags: review?(geoff)
Comment on attachment 9123347 [details] [diff] [review]
part1-2-Fix-calendar-l10n-packaging-0.patch

Review of attachment 9123347 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable and appears to work for packaging and broader l10n purposes.
Attachment #9123347 - Flags: review?(paul) → review+
Attachment #9123349 - Flags: review?(philipp)
Attachment #9123349 - Flags: review?(geoff)
Attachment #9123349 - Flags: review+
Comment on attachment 9123350 [details] [diff] [review]
part3-Update-paths-for-components-and-other-js-json-files-1.patch

Review of attachment 9123350 [details] [diff] [review]:
-----------------------------------------------------------------

I predict the calExtract module is broken outside of en-US, but then I think that's already the case. It depends on other locales being available, but they aren't.

::: calendar/base/src/calTimezoneService.js
@@ +133,5 @@
>          cal.LOG("[calTimezoneService] Timezones version " + this.version + " loaded");
>  
> +        let bundleUrl = hasTimezonesAddon
> +          ? "chrome://calendar-timezones/locale/timezones.properties"
> +          : "resource:///chrome/en-US/locale/en-US/calendar/timezones.properties";

chrome://calendar/locale/timezones.properties
Attachment #9123350 - Flags: review?(philipp)
Attachment #9123350 - Flags: review?(geoff)
Attachment #9123350 - Flags: review+
Attachment #9123359 - Flags: review?(geoff) → review+
Comment on attachment 9123383 [details] [diff] [review]
part34-Update-paths-in-allowed-dupes-mn-0.patch

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.
Attachment #9123383 - Flags: review?(philipp)
Attachment #9123383 - Flags: review?(geoff)
Attachment #9123383 - Flags: review-
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.

Leaving Philipp's r? for his opinion too.
Attachment #9123384 - Flags: review?(geoff) → review+
Attachment #9123385 - Flags: review?(philipp)
Attachment #9123385 - Flags: review?(geoff)
Attachment #9123385 - Flags: review+
Comment on attachment 9123346 [details] [diff] [review]
part1-1-Change-build-system-to-integrate-calendar-1.patch

Yes, but `FINAL_TARGET_FILES['components']` could be `FINAL_TARGET_FILES.components` in several places.
Attachment #9123346 - Flags: review?(philipp)
Attachment #9123346 - Flags: review?(geoff)
Attachment #9123346 - Flags: review+

FINAL_TARGET_FILES['components'] -> FINAL_TARGET_FILES.components

Attachment #9123346 - Attachment is obsolete: true
Attachment #9123432 - Flags: review+

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

Attachment #9123433 - Flags: review?(geoff)

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

Attachment #9123350 - Attachment is obsolete: true
Attachment #9123434 - Flags: review+

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).

Attachment #9123383 - Attachment is obsolete: true
Attachment #9123435 - Flags: review?(geoff)
Blocks: 1612198

(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.

Blocks: 1599212

Okay, I see, that makes sense.

Blocks: 1612166
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.
Attachment #9123433 - Flags: review?(geoff)
Depends on: 1612168
Attachment #9123435 - Flags: review?(geoff) → review+
Comment on attachment 9124066 [details] [diff] [review]
part3-1-Remove-code-for-the-calendar-timezones-addon-1.patch

Please ignore, uploaded incomplete patch by accident.
Attachment #9124066 - Attachment is obsolete: true
Attachment #9124066 - Flags: review?(geoff)

(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.

Attachment #9123433 - Attachment is obsolete: true
Attachment #9124101 - Flags: review?(geoff)
Blocks: 1612168
No longer depends on: 1612168

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

Attachment #9124101 - Flags: review?(geoff) → review+
Comment on attachment 9123384 [details] [diff] [review]
part35-On-startup-uninstall-Lightning-addon-if-installed-0.patch

Review of attachment 9123384 [details] [diff] [review]:
-----------------------------------------------------------------

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!
Attachment #9123384 - Flags: review?(philipp) → review+
Attachment #9123347 - Flags: review?(philipp) → review+

(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
  1. 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.

  2. 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))
  1. 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.

Attachment #9124642 - Flags: review?(rob)
Comment on attachment 9124642 [details] [diff] [review]
part0-lightning-taskcluster-1.diff

Review of attachment 9124642 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #9124642 - Flags: review?(rob) → review+

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

Pushed by paul@paulwmorris.com:
https://hg.mozilla.org/comm-central/rev/0d533b4b3992
Remove various Lightning pieces from TaskCluster configuration. r=rjl
https://hg.mozilla.org/comm-central/rev/91a334ef09a8
Change build system to integrate calendar. r=darktrojan,Fallen
https://hg.mozilla.org/comm-central/rev/ba5a124ff8bb
Fix calendar l10n packaging. r=pmorris
https://hg.mozilla.org/comm-central/rev/340a38d16c2e
Update 'modules' paths. r=darktrojan
https://hg.mozilla.org/comm-central/rev/5802fba79ad0
Use ChromeUtils.defineModuleGetter where possible. r=darktrojan
https://hg.mozilla.org/comm-central/rev/617509326044
Remove code for the calendar-timezones addon. r=darktrojan
https://hg.mozilla.org/comm-central/rev/50ae36bb50fd
Update paths for components and other js/json files. r=darktrojan
https://hg.mozilla.org/comm-central/rev/8d524b01c605
Integrate messenger-overlay-accountCentral.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/1fa6810ac522
Integrate messenger-overlay-messageWindow.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/8f7bf0b238cb
Integrate lightning-item-panel.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/dca41d376996
Integrate messenger-overlay-preferences.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/13a1ac7c5683
Show "Calendar" sidebar item on preferences page. r=darktrojan
https://hg.mozilla.org/comm-central/rev/0fe3458c274c
Integrate imip-bar-overlay.inc.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/4656ef92d6aa
Fix 'msgHeaderView element is null' error. r=Fallen
https://hg.mozilla.org/comm-central/rev/873146a4a36c
Inline the DTD/CSS/JS files from messenger-overlay-sidebar. r=Fallen
https://hg.mozilla.org/comm-central/rev/05e3a9199721
Pre-process calendar commands into messenger.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/05f8690b4ae9
Pre-process calendar keys into messenger.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/b959058157bc
Pre-process calendar context menus into messenger.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/1331b8f4fdfb
Inline calendar tab bar buttons into messenger.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/e397711a34f5
Pre-process calendar tab panels into messenger.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/37bca8ac52ca
Pre-process calendar today pane into messenger.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/65d67acb5b16
Pre-process calendar status bar into messenger.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/df845a3dbec6
Pre-process calendar buttons for mail toolbar. r=Fallen
https://hg.mozilla.org/comm-central/rev/001eb334f82a
Pre-process calendar 'events and tasks' menu. r=Fallen
https://hg.mozilla.org/comm-central/rev/5ab844270ca1
Inline calendar file open and save menu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/0fe506be8cca
Inline calendar file new menu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/822561223c27
Inline calendar edit and go menu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/9a50d5b205f2
Pre-process calendar view menu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/08a88ac148a4
Inline 'convert calendar' context menu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/89181842d4a5
Inline calendar 'new' and 'events and tasks' appmenu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/e012cf345b6d
Inline calendar file->open appmenu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/5964bccb8f7b
Inline calendar 'view' and 'go' appmenu items. r=Fallen
https://hg.mozilla.org/comm-central/rev/106676f18f20
Pre-process calendar appmenu panelviews. r=Fallen
https://hg.mozilla.org/comm-central/rev/30b1085f2a92
Remove seamonkey/suite lines from jar.mn. r=Fallen
https://hg.mozilla.org/comm-central/rev/b27c5d446d95
Inline calendar CSS files from jar.mn files. r=Fallen
https://hg.mozilla.org/comm-central/rev/20890131209c
Remove XPCShell function that loads Lightning XPI. r=Fallen
https://hg.mozilla.org/comm-central/rev/cd731183002b
Add missing DTDs to messageWindow.xhtml. r=Fallen
https://hg.mozilla.org/comm-central/rev/f18c944f7f0e
Update paths in allowed-dupes.mn. r=darktrojan
https://hg.mozilla.org/comm-central/rev/81c1b4bf19a2
On startup uninstall Lightning addon if installed. r=darktrojan
https://hg.mozilla.org/comm-central/rev/e49a12783ad3
Fix error 'gDataMigrator not defined'. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 74.0
Regressions: 1614194
Regressions: 1615124
Regressions: 1615453
Regressions: 1615700
Regressions: 1615714
Regressions: 1620123
You need to log in before you can comment on or make changes to this bug.