1481790 - Get rid of loading localized default preferences from lightning-l10n.js file

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[[:MakeMyDay]]

In lightning-l10n.js [1] we currently define some date related default values for preferences:

# the default day to start the week on
#0=Sunday 1=Monday 2=Tuesday 3=Wednesday 4=Thursday 5=Friday 6=Saturday
pref("calendar.week.start", 0);

# default days off (not in work week)
pref("calendar.week.d0sundaysoff", true);
pref("calendar.week.d1mondaysoff", false);
pref("calendar.week.d2tuesdaysoff", false);
pref("calendar.week.d3wednesdaysoff", false);
pref("calendar.week.d4thursdaysoff", false);
pref("calendar.week.d5fridaysoff", false);
pref("calendar.week.d6saturdaysoff", true);

Instead of relying on that, which causes problem when switching between single and multi-locale versions, we detect the date related prefs by using

Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl).getCalendarInfo(locale);

This provides e.g. for de-DE

{ firstDayOfWeek: 2, minDays: 4, weekendStart: 7, weekendEnd: 1, calendar: "gregory", locale: "de-DE" }

With firstDayOfWeek we have a replacement for calendar.week.start, the weekendStart/weekendEnd would would allow to replace calendar.week.*

On top of this, we can use minDays to calculate the first week of the year properly.

I suggest to implement this asap to be able to ship it with the next minor TB ESR release, when people would switch back from multi to single locale (if we have the ATN out before). Philipp, what do you think?

That would just leave the localized categories for lightning-l10n.js

[1] https://searchfox.org/comm-central/source/calendar/locales/en-US/lightning-l10n.js

Comment 1

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

I'm fine with this approach. As for the categories, do we really need the l10n prefs file for that? A quick check shows we already have a l10n string for those (categories2).

Comment 2

[[:MakeMyDay]]

We can move that, too. Patch is coming, I just discovered one issue when testing the patch from bug 1473294.

Comment 3

[[:MakeMyDay]]

Attachment: DetermineLocaleSensitvePreferenceDefaultsWithoutLightningL10nJs-V1.diff

This patch determines the locale sensitive default values for preferences by using the intl service for the first day of the week and the weekend indicators. The catagory names get loaded from the categories2 string.

I intentionally made this to overload the default values instead of changing that for all the consumers of these preferences to minimize the risk for taking this for esr60.

This can be optimized if required with follow-up bugs for 63+.

Bug 1473294 is not obsoleted by this bug to make it right for other addons and should at best land before this, otherwise it will be difficult to verify that one works except we know another addon that uses an l10n default preferences file.

Comment 4

[[:MakeMyDay]]

Attachment: RemoveLigtningL10nJs-V1.diff

This takes care to remove lightning-l10n.js from the tree. I separated this to ease testing the patch.

When uplifting the other patch to esr60, uplifting this one is technically not needed but would avoid an extra load.

Comment 5

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

Comment on attachment 8999176 [details] [diff] [review]
DetermineLocaleSensitvePreferenceDefaultsWithoutLightningL10nJs-V1.diff

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

::: calendar/base/content/calendar-chrome-startup.js
@@ +248,5 @@
> +            let weekend = [];
> +            for (let i = weStart; i <= weEnd; i++) {
> +                weekend.push(i > 6 ? i - 7 : i);
> +            }
> +            let dayNumber = aName.substr(15, 1);

Isn't this just aName[15] ?

@@ +255,5 @@
> +            }
> +        }
> +    }
> +
> +    cal.LOG("Start loading of locale sensitive preference default values...");

I think sensitive is the wrong word here, did you mean dependent?

::: calendar/base/modules/utils/calL10NUtils.jsm
@@ +169,5 @@
> +     * @param {String} aLocale     The locale to get the info for, e.g. "en-US",
> +     *                               "de-DE" or null for the current locale
> +     * @return {Object}            The getCalendarInfo object from mozIMozIntl
> +     */
> +    calendarInfo: _calendarInfo

If this is just used once, you could also use an IIFE to attach the cache and then return calendarInfo. It would keep the function together with where it is exported.

::: calendar/test/unit/test_call10nutils.js
@@ +25,5 @@
> +        input: { locale: "en-US" },
> +        expected: {
> +            properties: [
> +                "firstDayOfWeek", "minDays", "weekendStart", "weekendEnd",
> +                "calendar", "locale"]

Is eslint happy with this? Would probably have used

{
    properties: [
        "a", ...,
        "b", ...
    ]
}

::: calendar/test/unit/xpcshell-shared.ini
@@ +16,5 @@
>  [test_bug523860.js]
>  [test_bug653924.js]
>  [test_bug668222.js]
>  [test_bug759324.js]
> +[test_call10nutils.js]

in line with test_view_utils, maybe just test_l10n_utils?

Comment 6

[[:MakeMyDay]]

Attachment: DetermineLocaleDependentPreferenceDefaultsWithoutLightningL10nJs-V2.diff

Updated patch with most comments considered (though the linter was satisfied either way). I didn't switch to an IIFE since when starting to use this also to determine the first week of the year, we will need to observe pref changes and reset the cache either entirely or at least for that property.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c451efd60c38
Remove lightning-l10n-js. r=philipp
https://hg.mozilla.org/comm-central/rev/6d879599a126
Determine locale dependent default values for preferences without a localized preference file. r=philipp

Comment 8

[Jorg K (CEST = GMT+2)]

Sadly your test is failing on the tree on Mac, passed on Linux, no result on Windows yet.
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:comm/calendar/test/unit/test_l10n_utils.js | calendarInfo_test - [calendarInfo_test : 100] caching retest - value for locale is different in both objects (test #4) - "en" != "en"

I ran it on Windows locally before landing, and it failed when my regional settings were set to en-GB, worked OK with en-US.

Comment 9

[[:MakeMyDay]]

Attachment: FixCalL10NTest-V1.diff

debug patch

Comment 10

[Jorg K (CEST = GMT+2)]

You should have started a try with this since trunk it busted since 11:45 :-(

You can still do so by pointing .taskcluster.yml (GECKO_HEAD_REF: 'default') to a non-busted M-C version, like 49b70f7e6817.

Comment 11

[[:MakeMyDay]]

Already running: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d883a38343897507c4841024144d8d7045ed184

Comment 12

[Jorg K (CEST = GMT+2)]

Good, you're called Geoff now ;-)

Comment 13

[[:MakeMyDay]]

Attachment: FixCalL10NTest-V2.diff

another debug patch.

Comment 14

[[:MakeMyDay]]

Attachment: PartlyDisableCalL10NTest-V1.diff

We have two issue with the test here.

A. Services.intl.getCalendarInfo() seems to provide caches values if the language part of the locale doesn't change (e.g. from en-GB to en-US). Not sure whether this is an issue or not, needs further investigation.

B. Cc["@mozilla.org/intl/ospreferences;1"].getService(Ci.mozIOSPreferences).getRegionalPrefsLocales() provides only "en" on macOS while "en-US" on other OS, while the appLocale is always reported as "en-US", so comparing appLocale and rsLocale in the test code is not enough when it comes to the cache reset test.

Since we currently don't reset our cache at runtime atm anyway, this patch disables the offending test step. With that patch applied, we can safely go forward to 60.1 and fix the failing test in a follow up bug.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003776 [details] [diff] [review]
PartlyDisableCalL10NTest-V1.diff

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

::: calendar/test/unit/test_l10n_utils.js
@@ +98,3 @@
>              Preferences.set("intl.regional_prefs.use_os_locales", useOSLocaleFormat);
> +            // This is currently since the code actually doesn't reset the cache anyway
> +            // when re-enabling, be aware that macos return just "en" for rsLocale while other

Would you mind fixing the English here:
// This is currently ??? since the code actually doesn't reset the cache anyway.
// When re-enabling, be aware that macOS returns just "en" for rsLocale while other

Comment 16

[[:MakeMyDay]]

??? = disabled - if want this to be fixed, please do so when pushing, I'm afk for probably the rest of today.

Comment 17

[Jorg K (CEST = GMT+2)]

OK, will do. We're busted anyway right now.

Comment 18

[Jorg K (CEST = GMT+2)]

TB 60 beta 11 / Cal 6.2:
https://hg.mozilla.org/releases/comm-beta/rev/bfe40785104956eda8625dc566695dcb318676f9
https://hg.mozilla.org/releases/comm-beta/rev/52574f7002f0c3fed0d7486b4dbfbf0d5589da99
https://hg.mozilla.org/releases/comm-beta/rev/5a5c21ea76a619f9cfef109fcf3308f94a52c5c4

Last changeset not landed on C-C yet.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b786d0165808
Disable test steps for testing cache resetting for calendarInfo. r=philipp

Comment 20

[Jorg K (CEST = GMT+2)]

TB 60.1 ESR / Cal 6.2.1:
https://hg.mozilla.org/releases/comm-esr60/rev/ca7a6c6a69703aad7aa2c2730b6d9a6f85332a02
https://hg.mozilla.org/releases/comm-esr60/rev/f85b7f6cbe59f7322c231f9c39d0d311193f3552
https://hg.mozilla.org/releases/comm-esr60/rev/a69b790612e784ce50380a448d65f5caa6b974c3