1420173 - Make L10nRegistry handle source/locale versions

Status: NEW

(Core :: Internationalization, enhancement, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

When we initially rolled out L10nRegistry, we didn't handle the concept of locale versions. When we landed new language packs, we added a way to annotate each locale resource with a version based on the push timestamp to each l10n-central/AB-CD repo that we use for building that langpack.

This bug is about connecting those two.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1420173 - Filter and sort sources by version in L10nRegistry.

Review commit: https://reviewboard.mozilla.org/r/202538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202538/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8931401 [details]
Bug 1420173 - Filter and sort sources by version in L10nRegistry.

In the spirit of trying to get :stas involved as a reviewer of intl/l10n, setting r? on him, but still an f? on :mossop who reviewed the whole L10nRegistry.

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 8931401 [details]
Bug 1420173 - Filter and sort sources by version in L10nRegistry.

https://reviewboard.mozilla.org/r/202538/#review207898

::: intl/l10n/L10nRegistry.jsm:98
(Diff revision 1)
>      }
> -    const sourcesOrder = Array.from(this.sources.keys()).reverse();
>      for (const locale of requestedLangs) {
> +      const sourcesOrder = Array.from(this.sources.keys()).filter(name => {
> +        return this.sources.get(name).locales.has(locale);
> +      }).reverse().sort((a, b) => {

This is cosmetic but have you considered `[...this.sources.keys()]` instead of `Array.from`? Also the filter function doesn't need the braces (nor the explicit return statement).

Does reversing the array help sorting in any way?

::: intl/l10n/L10nRegistry.jsm:144
(Diff revision 1)
>     *
>     * @param {String} sourceId
>     */
>    removeSource(sourceName) {
>      this.sources.delete(sourceName);
> +    this.ctxCache.clear();

Is this related or rather a drive-by fix?

::: intl/l10n/L10nRegistry.jsm:289
(Diff revision 1)
>     */
>    constructor(name, locales, prePath) {
>      this.name = name;
> -    this.locales = locales;
> +    this.locales = Array.isArray(locales) ?
> +      new Map(locales.map(loc => [loc, 0])) :
> +      new Map(Object.entries(locales));

Do we want to support both versioned and unversioned locales in the long term?

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Does reversing the array help sorting in any way?

Yes. When we add sources in `L10nRegistry.registerSource` we store them in insertion order (Map preserves the order). So if two sources have the same version we'll return ['older', 'newer'] unless we reverse it.

I believe that if we ever notice that L10nRegistry starts showing up in profiles, we can move more of the ordering logic to the registerSource piece, but for now, I think those operations should be fast as they'll almost always operate on a 1-3 element lists.

> Is this related or rather a drive-by fix?

I noticed it while testing this patch, but it's a drive-by fix.

> Do we want to support both versioned and unversioned locales in the long term?

At least until we fix bug 1420236 to get versions for packaged locales.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8931401 [details]
Bug 1420173 - Filter and sort sources by version in L10nRegistry.

https://reviewboard.mozilla.org/r/202538/#review208664

::: intl/l10n/L10nRegistry.jsm:21
(Diff revision 1)
>   *
>   * Example:
>   *
>   *   FileSource1:
>   *     name: 'app'
> - *     locales: ['en-US', 'de']
> + *     locales: {'en-US': '201701010000', 'de': '20170101123500'}

This suggests that the locale versions are strings whereas they are numbers.

::: intl/l10n/L10nRegistry.jsm:280
(Diff revision 1)
>   * come from the cache.
>   **/
>  class FileSource {
>    /**
> -   * @param {string}         name
> -   * @param {Array<string>}  locales
> +   * @param {string}                                name
> +   * @param {Object<string, number>|Array<string>}  locales

Should we do verification that the versions are in fact numbers? Otherwise you're going to get odd results when sorting them.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8931401 [details]
Bug 1420173 - Filter and sort sources by version in L10nRegistry.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202538/diff/1-2/

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks :stas, :mossop!

Updated the patch to your feedback.