1626066 - Re-organize code in messenger-overlay-sidebar.js

Status: RESOLVED FIXED

(Calendar :: General, task, P3)

Description

[Paul Morris [:pmorris]]

The code for setting up calendar functionality is split up into two places messenger-overlay-sidebar.js and calendar-chrome-startup.js (due to the historical lightning/base split), and messenger-overlay-sidebar.js has become a catch-all location for code that should really be organized into separate files.

Before modifying the initialization code so that background calendar services etc. are started/stopped when the calendar component is activated/deactivated (in bug 1623111), I've re-organized the code in messenger-overlay-sidebar.js, moving all of the initialization code into calendar-chrome-startup.js and moving other code out into separate files where that makes sense.

Comment 1

[Paul Morris [:pmorris]]

Attachment: part1-last-shown-calendar-view-global-0.patch

Part 1 of 6: Replace gLastShownCalendarView global variable with an object.

If we want to use it in more than one file then it needs to become an eslint global variable, but then it cannot be changed according to eslint rules. But if it's a global object we can call its methods without breaking any rules.

Comment 2

[Paul Morris [:pmorris]]

Attachment: part2-move-cal-tabs-code-out-0.patch

Part 2 of 6: Move calendar tabs code out of messenger-overlay-sidebar.js

Comment 3

[Paul Morris [:pmorris]]

Attachment: part3-move-cal-modes-code-out-0.patch

Part 3 of 6: Move calendar modes code out of messenger-overlay-sidebar.js

Comment 4

[Paul Morris [:pmorris]]

Attachment: part4-move-invitations-manager-out-0.patch

Part 4 of 6: Move calendar invitations manager code out of messenger-overlay-sidebar.js

Comment 5

[Paul Morris [:pmorris]]

Attachment: part5-combine-load-functions-0.patch

Part 5 of 6: Combine previously separate calendar load (and unload) functions

Comment 6

[Paul Morris [:pmorris]]

Attachment: part6-merge-into-calendar-chrome-startup-0.patch

Part 6 of 6: Merge (remaining code from) messenger-overlay-sidebar.js into calendar-chrome-startup.js

Comment 7

[Paul Morris [:pmorris]]

Attachment: part2-move-cal-tabs-code-out-1.patch

Part 2 of 6: Move calendar tabs code out of messenger-overlay-sidebar.js

Updated to fix eslint import-globals-from comments. (Originally I was keeping the new files in /lightning/, but then moved them to /base/ and missed updating these comments.)

Comment 8

[Paul Morris [:pmorris]]

Attachment: part3-move-cal-modes-code-out-1.patch

Part 3 of 6: Move calendar modes code out of messenger-overlay-sidebar.js

Updated to fix eslint import-globals-from comments. (Originally I was keeping the new files in /lightning/, but then moved them to /base/ and missed updating these comments.)

Comment 9

[Paul Morris [:pmorris]]

Try run that revealed the linting problem: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=75e1b9771aaf60c1c1b835c0d4615de9a3712433

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9136921 [details] [diff] [review]
part2-move-cal-tabs-code-out-1.patch

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

::: mail/base/content/hiddenWindow.xhtml
@@ +75,5 @@
>  <script src="chrome://lightning/content/lightning-utils.js"/>
>  <script src="chrome://lightning/content/imip-bar.js"/>
>  <script src="chrome://calendar/content/calendar-management.js"/>
>  <script src="chrome://calendar/content/calendar-ui-utils.js"/>
> +<script src="chrome://calendar/content/calendar-tabs.js"/>

I'll allow this as long as you make a follow-up bug to go through all these script tags and reduce them to the minimum necessary.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9136898 [details] [diff] [review]
part5-combine-load-functions-0.patch

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

Debatable how much of this stuff (especially on unload) actually needs to happen in this way now that we're not an extension. Some of it probably should not happen a second time if we open a new window. (I'm sure I filed a bug on that years ago but I cannot find it.)

But anyway, this is good enough for now.

Comment 12

[Geoff Lankow (:darktrojan)]

Comment on attachment 9136901 [details] [diff] [review]
part6-merge-into-calendar-chrome-startup-0.patch

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

::: calendar/base/content/calendar-chrome-startup.js
@@ +483,5 @@
> +/**
> + * Called at midnight to tell us to redraw date-specific widgets.  Do NOT call
> + * this for normal refresh, since it also calls scheduleMidnightUpdate.
> + */
> +function refreshUIBitsAtMidnight() {

Let's get rid of the "Bits", since we're renaming it anyway.

Comment 13

[Geoff Lankow (:darktrojan)]

I haven't looked too closely at most of this since it's just moving code from one place to another. Overall it's heading in the right direction.

Looking ahead, I'd like to dramatically reduce the number of members we're adding to window by grouping similar things into objects. Kinda like the part 1 patch here, but with all of the things for dealing with the view in one object.

Comment 14

[Paul Morris [:pmorris]]

Attachment: part6-merge-into-calendar-chrome-startup-1.patch

Thanks for the review. I've renamed that function to "doMidnightUpdate" to match "scheduleMidnightUpdate" and to drop the "bits".

Agreed that there's a lot more that could be done, grouping things into objects, etc., and this is just an incremental step towards where we want to go. I'll open a follow-up bug for the hiddenWindow.xhtml scripts. Maybe we can make progress on loading/unloading in the next step, bug 1623111.

Comment 15

[Paul Morris [:pmorris]]

Final try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5acbc23e43e2be6dd67fd9ea906ed0b7631c2c8e

Comment 16

[Worcester12345]

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

Comment on attachment 9136898 [details] [diff] [review]
part5-combine-load-functions-0.patch

Review of attachment 9136898 [details] [diff] [review]:

Debatable how much of this stuff (especially on unload) actually needs to
happen in this way now that we're not an extension. Some of it probably
should not happen a second time if we open a new window. (I'm sure I filed a
bug on that years ago but I cannot find it.)

But anyway, this is good enough for now.

https://bugzilla.mozilla.org/show_bug.cgi?id=1471831

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7b90d0e94fa3
Replace gLastShownCalendarView global variable with an object. r=darktrojan
https://hg.mozilla.org/comm-central/rev/e9593a4b546c
Move calendar tabs code out of messenger-overlay-sidebar.js. r=darktrojan
https://hg.mozilla.org/comm-central/rev/3659c628e591
Move calendar modes code out of messenger-overlay-sidebar.js. r=darktrojan
https://hg.mozilla.org/comm-central/rev/4c4e4de548fa
Move calendar invitations manager code out of messenger-overlay-sidebar.js. r=darktrojan
https://hg.mozilla.org/comm-central/rev/51af2ea087fc
Combine previously separate calendar load (and unload) functions. r=darktrojan
https://hg.mozilla.org/comm-central/rev/b4dc69d562b2
Merge messenger-overlay-sidebar.js into calendar-chrome-startup.js. r=darktrojan