1408662 - Move date/time/timezone related functions into calDateTimeUtils.jsm

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'm proposing to move the following functions into their own namespace, with some slight renames to remove redundacy.

cal.now -> cal.dtz.now
cal.ensureDateTime -> cal.dtz.ensureDateTime
cal.getRecentTimezones -> cal.dtz.getRecentTimezones
cal.saveRecentTimezone -> cal.dtz.saveRecentTimezone
cal.getDefaultStartDate -> cal.dtz.getDefaultStartDate
cal.setDefaultStartEndHour -> cal.dtz.setDefaultStartEndHour
cal.calGetStartDateProp -> cal.dtz.startDateProp
cal.calGetEndDateProp -> cal.dtz.endDateProp
cal.sameDay -> cal.dtz.sameDay
cal.jsDateToDateTime -> cal.dtz.jsDateToDateTime
cal.dateTimeToJsDate -> cal.dtz.dateTimeToJsDate
cal.calendarDefaultTimezone -> cal.dtz.defaultTimezone
cal.floating -> cal.dtz.floating
cal.UTC -> cal.dtz.UTC


To decrease the chance of regressions I've decided to go with a scripted approach to do the renames. Therefore, for review, I am going to attach:
1) A patch with manual steps, e.g. moving the functions into their own module
2) The script I used to do the automatic changes
3) A patch with only the automatic steps.

When I have r+ for (1) and (2), I will upload a combined patch (1+3) for checkin, as they obviously don't work without each other. You can give (2) a quick glance, but I don't think it is useful to spend time on each line in that change. Just make sure the script covers all edge cases :)

** Open questions I am not set on **
* cal.dtz.defaultTimezone, cal.dtz.UTC and cal.dtz.floating are currently functions, just as they were before the move. These could also be readonly getters which might make more sense. Do you agree?

* The current mechanism requires callers to always load calUtils.jsm, never calDateTimeUtils.jsm. Access is always via cal.dtz.*. I like this because of "namespacing", but it may (or may not) have adverse effects on performance or memory usage. There will be some circular dependencies I've resolved through lazy loading. Is what I am doing here reasonable, or should we separate out anything?

* Adding to the previous item, possibly we should put these sub-modules into calendar/base/modules/calutils/dtz.jsm and put a big fat warning at the top that it should not be loaded separately. Thoughts?

* This will break addon compatibility quite badly. I don't want to leave in the old functions because this is also about cleanup. The question is if we want to provide a deprecated wrapper for these functions, or just expect authors to update their code. The gdata code will have a shim that add-on authors can use, but they really should change their code instead (maybe use the script (2) as well). Thoughts?

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Manual Changes - v1

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Script to rename functions - v1

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Automatic Changes - v1

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Manual Changes - v2

Fixed Indent

Comment 5

[[:MakeMyDay]]

Thanks for getting to this. When moving the functions around, I would appreciate to also consolidate the unit tests accordingly and add tests for those dtz functions that are missing tests, if any.

(In reply to Philipp Kewisch [:Fallen] from comment #0)
> ** Open questions I am not set on **
> * cal.dtz.defaultTimezone, cal.dtz.UTC and cal.dtz.floating are currently
> functions, just as they were before the move. These could also be readonly
> getters which might make more sense. Do you agree?

Yes, these should be readonly getters - version and defaultTimezone are other candidates.
 
> * The current mechanism requires callers to always load calUtils.jsm, never
> calDateTimeUtils.jsm. Access is always via cal.dtz.*. I like this because of
> "namespacing", but it may (or may not) have adverse effects on performance
> or memory usage. There will be some circular dependencies I've resolved
> through lazy loading. Is what I am doing here reasonable, or should we
> separate out anything?

What circular dependencies do you get especcialy? If that are not too much, I would be in favour to separate them out and follow the same way of module inclusion like we do for other modules extending the cal namespace directly.

> * Adding to the previous item, possibly we should put these sub-modules into
> calendar/base/modules/calutils/dtz.jsm and put a big fat warning at the top
> that it should not be loaded separately. Thoughts?

That way we would end up to move nearly all modules in the subfolder, as most of them extend the cal namespace. Or are you referring to the lazy loaded one? I think we can spare that extra folder. 

> * This will break addon compatibility quite badly. I don't want to leave in
> the old functions because this is also about cleanup. The question is if we
> want to provide a deprecated wrapper for these functions, or just expect
> authors to update their code. The gdata code will have a shim that add-on
> authors can use, but they really should change their code instead (maybe use
> the script (2) as well). Thoughts?

If you don't want to go the way to leave the old functions in place for logging a deprecate warning and redirect to the new place, we should fail hard, maybe with an additional warning referencing the change.

Regardless, an announcement to dev.apps.calendar (and probably also to the authors of the most used addons directly) describing the ways to fix this would be nice. And we should either get this bug fixed and landed quite soon or postpone it beyond the ESR 59 release to avoid a flood of bug reports and force last minute changes on the addon maintainers.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to [:MakeMyDay] from comment #5)
> Thanks for getting to this. When moving the functions around, I would
> appreciate to also consolidate the unit tests accordingly and add tests for
> those dtz functions that are missing tests, if any.
> 
You are right we need this, but it is quite an ask. The patches I have in the pipeline are fairly massive by themselves, and making sure all functions have unit tests would be a major effort. I'll try to do this, but it will be a separate patch, likely on a separate bug blocking the meta bug.


>  
> > * The current mechanism requires callers to always load calUtils.jsm, never
> > calDateTimeUtils.jsm. Access is always via cal.dtz.*. I like this because of
> > "namespacing", but it may (or may not) have adverse effects on performance
> > or memory usage. There will be some circular dependencies I've resolved
> > through lazy loading. Is what I am doing here reasonable, or should we
> > separate out anything?
> 
> What circular dependencies do you get especcialy? If that are not too much,
> I would be in favour to separate them out and follow the same way of module
> inclusion like we do for other modules extending the cal namespace directly.
I don't quite remember the details. What I do remember is that it was some date time function which required a function from another module, which in turn used date/time functions. Possibly the use of cal.isEvent() which is later cal.item.isEvent(), in calItemUtils, which also uses date/time functions.

Of course I could split these out, but as for naming we'd probably end up with calItemUtils.jsm and calMoreItemUtils.jsm or something similarly strange. Maybe it makes sense to keep it as is and revisit when we've pushed all the patches and it is easier to see the end result.


> 
> > * Adding to the previous item, possibly we should put these sub-modules into
> > calendar/base/modules/calutils/dtz.jsm and put a big fat warning at the top
> > that it should not be loaded separately. Thoughts?
> 
> That way we would end up to move nearly all modules in the subfolder, as
> most of them extend the cal namespace. Or are you referring to the lazy
> loaded one? I think we can spare that extra folder. 
I was referring to "nearly all". We do have some modules that are loaded directly while the main batch (maybe a dozen short) would be loaded via calUtils.jsm. The idea behind this was to make it very obvious which ones can/should be loaded directly. 

> 
> > * This will break addon compatibility quite badly. I don't want to leave in
> > the old functions because this is also about cleanup. The question is if we
> > want to provide a deprecated wrapper for these functions, or just expect
> > authors to update their code. The gdata code will have a shim that add-on
> > authors can use, but they really should change their code instead (maybe use
> > the script (2) as well). Thoughts?
> 
> If you don't want to go the way to leave the old functions in place for
> logging a deprecate warning and redirect to the new place, we should fail
> hard, maybe with an additional warning referencing the change.
It would currently fail hard given the functions no longer exist on the cal. namespace. I don't think there are any backwards incompatible changes. I guess I could live with a wrapper that marks them deprecated for a few cycles, I'll make this a separate jsm that is automatically loaded and modifies the cal namespace. This way it is easier to throw it away later on.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Script to rename functions - v2

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Manual Changes - v3

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Manual Changes - v4

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Automatic Changes - v2

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

 https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b7f9db8f15fb7867aaa83548d525692328db1b69

Comment 12

[[:MakeMyDay]]

Can you pleasr trigger a new try build now that bug 1424350 is fixed and all unit tests are back?

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f84fbfc142bd84f015453aa5743748c29338ae22

Comment 14

[[:MakeMyDay]]

Comment on attachment 8942468 [details] [diff] [review]
Manual Changes - v4

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

Thanks, r+ with the comments considered and provided a green try push (including eslint) for this and the automated changes.

::: calendar/base/modules/calUtils.jsm
@@ +1006,5 @@
>  // Some functions in calUtils.js refer to other in the same file, thus include
>  // the code in global scope (although only visible to this module file), too:
>  cal.loadScripts(["calUtils.js"], Components.utils.getGlobalForObject(cal));
> +
> +// Backwards compatibility for bug 905097. Please remove with Thunderbird 60.

Please make this 61, since ESR is 60 now. Can you please also file a bug for this once the patch landed?

::: calendar/base/modules/calUtilsCompat.jsm
@@ +32,5 @@
> +        UTC: "UTC"
> +    }
> +};
> +
> +function generateForward(global, namespace, from, to) {

Do you want to remove this file again in TB61, too? If not, please add doc blocks for the functions.

::: calendar/base/src/calUtils.js
@@ +676,5 @@
>   */
>  function checkIfInRange(item, rangeStart, rangeEnd, returnDtstartOrDue) {
>      let startDate;
>      let endDate;
> +    let queryStart = cal.ensureDateTime(rangeStart);

shouldn't this be cal.dtz.ensureDateTime? Applies for all occurences in this file.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I've fixed the comments and made another try push, that should work this time:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=976a9873f593b7e475393e49d1f918af40d99db8

Regarding cal.dtz.ensureDateTime, this is fixed as part of the automatic changes patch. The manual changes are only those that I couldn't (easily) automate.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Blocking on bug 1431913, reluctant to run a try run before that patch actually works.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

The band-aid for that bug landed, so I've started another try run:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=de40a92c8986530ee182e01d041ce1f359306044

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Manual Changes - v5

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Automatic Changes - v3

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

For checkin, please push the manual changes v5 patch, then automatic changes v3, in that order.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/015d6cb840c4
Move date/time/timezone related functions into calDateTimeUtils.jsm - manual changes. r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/18938fc0dd70
Move date/time/timezone related functions into calDateTimeUtils.jsm - automatic changes. r=MakeMyDay