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 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 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?

> 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 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.

Thanks :stas, :mossop! Updated the patch to your feedback.