Closed Bug 1481790 Opened Last year Closed Last year

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

Categories

(Calendar :: Internal Components, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
6.2.2.1

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

Attachments

(3 files, 3 obsolete files)

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
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).
We can move that, too. Patch is coming, I just discovered one issue when testing the patch from bug 1473294.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Summary: Detect locale aware date preferences automatically instead of loading from lightning-l10n.js → Get rid of loading localized default preferences from lightning-l10n.js file
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.
Attachment #8999176 - Flags: review?(philipp)
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.
Attachment #8999177 - Flags: review?(philipp)
Blocks: ltn62
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?
Attachment #8999176 - Flags: review?(philipp) → review+
Attachment #8999177 - Flags: review?(philipp) → review+
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.
Attachment #8999176 - Attachment is obsolete: true
Attachment #9003374 - Flags: approval-calendar-esr?(philipp)
Attachment #9003374 - Flags: review+
Attachment #8999177 - Flags: approval-calendar-esr?(philipp)
Attachment #8999177 - Flags: approval-calendar-esr?(philipp) → approval-calendar-esr+
Attachment #9003374 - Flags: approval-calendar-esr?(philipp) → approval-calendar-esr+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.5
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(makemyday)
Resolution: FIXED → ---
Attached patch FixCalL10NTest-V1.diff (obsolete) β€” β€” Splinter Review
debug patch
Flags: needinfo?(makemyday)
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.
Good, you're called Geoff now ;-)
Attached patch FixCalL10NTest-V2.diff (obsolete) β€” β€” Splinter Review
another debug patch.
Attachment #9003739 - Attachment is obsolete: true
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.
Attachment #9003759 - Attachment is obsolete: true
Attachment #9003776 - Flags: review?(philipp)
Attachment #9003776 - Flags: approval-calendar-esr?(philipp)
Attachment #9003776 - Flags: review?(philipp)
Attachment #9003776 - Flags: review+
Attachment #9003776 - Flags: approval-calendar-esr?(philipp)
Attachment #9003776 - Flags: approval-calendar-esr+
Keywords: checkin-needed
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
??? = disabled - if want this to be fixed, please do so when pushing, I'm afk for probably the rest of today.
OK, will do. We're busted anyway right now.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b786d0165808
Disable test steps for testing cache resetting for calendarInfo. r=philipp
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 6.2 → 6.2.1
Target Milestone: 6.2.1 → 6.2.2
You need to log in before you can comment on or make changes to this bug.