Bug 1420173 - Filter and sort sources by version in L10nRegistry. r?stas draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Wed, 22 Nov 2017 07:20:35 -0300
changeset 703958 d8bc01e9211ece43a9a4e6d3dfb62356296189d3
parent 703363 4ce67352b2a9744ac51f0333cea0455e3d59bbf3
child 741942 2c4ec72030d1cffc02e098539660367f088d8b86
push id91010
push userbmo:gandalf@aviary.pl
push dateTue, 28 Nov 2017 00:02:57 +0000
reviewersstas
bugs1420173
milestone59.0a1
Bug 1420173 - Filter and sort sources by version in L10nRegistry. r?stas MozReview-Commit-ID: GE5WmaCXQu3
intl/l10n/L10nRegistry.jsm
intl/l10n/test/test_l10nregistry.js
toolkit/components/extensions/Extension.jsm
--- a/intl/l10n/L10nRegistry.jsm
+++ b/intl/l10n/L10nRegistry.jsm
@@ -13,24 +13,24 @@ Components.utils.importGlobalProperties(
  *
  * The generator creates all possible permutations of locales and sources to allow for
  * complete fallbacking.
  *
  * Example:
  *
  *   FileSource1:
  *     name: 'app'
- *     locales: ['en-US', 'de']
+ *     locales: {'en-US': 201701010000, 'de': 20170101123500}
  *     resources: [
  *       '/browser/menu.ftl',
  *       '/platform/toolkit.ftl',
  *     ]
  *   FileSource2:
  *     name: 'platform'
- *     locales: ['en-US', 'de']
+ *     locales: {'en-US': 201701010000, 'de': 20170101123500}
  *     resources: [
  *       '/platform/toolkit.ftl',
  *     ]
  *
  * If the user will request:
  *   L10nRegistry.generateContexts(['de', 'en-US'], [
  *     '/browser/menu.ftl',
  *     '/platform/toolkit.ftl'
@@ -87,18 +87,29 @@ const L10nRegistry = {
    * @param {Array} requestedLangs
    * @param {Array} resourceIds
    * @returns {AsyncIterator<MessageContext>}
    */
   async * generateContexts(requestedLangs, resourceIds) {
     if (this.bootstrap !== null) {
       await this.bootstrap;
     }
-    const sourcesOrder = Array.from(this.sources.keys()).reverse();
     for (const locale of requestedLangs) {
+      // We're performing three operations here:
+      //
+      // 1) Filtering the list of sources to only get ones that have the locale
+      // 2) Reversing in order to get the newer source first in case equal versions
+      // 3) Sort by locale version
+      const sourcesOrder = [...this.sources.keys()].filter(
+        name => this.sources.get(name).locales.has(locale)
+      ).reverse().sort((a, b) => {
+        const ver1 = this.sources.get(a).locales.get(locale);
+        const ver2 = this.sources.get(b).locales.get(locale);
+        return ver1 < ver2;
+      });
       yield * generateContextsForLocale(locale, sourcesOrder, resourceIds);
     }
   },
 
   /**
    * Adds a new resource source to the L10nRegistry.
    *
    * @param {FileSource} source
@@ -130,30 +141,31 @@ const L10nRegistry = {
 
   /**
    * Removes a source from the L10nRegistry.
    *
    * @param {String} sourceId
    */
   removeSource(sourceName) {
     this.sources.delete(sourceName);
+    this.ctxCache.clear();
     Services.obs.notifyObservers(null, 'l10n:available-locales-changed', null);
   },
 
   /**
    * Returns a list of locales for which at least one source
    * has resources.
    *
    * @returns {Array<String>}
    */
   getAvailableLocales() {
     const locales = new Set();
 
     for (const source of this.sources.values()) {
-      for (const locale of source.locales) {
+      for (const locale of source.locales.keys()) {
         locales.add(locale);
       }
     }
     return Array.from(locales);
   },
 };
 
 /**
@@ -264,25 +276,32 @@ function generateContext(locale, sources
  * It registers its own locales and a pre-path, and when asked for a file
  * it attempts to download and cache it.
  *
  * The Source caches the downloaded files so any consecutive loads will
  * come from the cache.
  **/
 class FileSource {
   /**
-   * @param {string}         name
-   * @param {Array<string>}  locales
-   * @param {string}         prePath
+   * @param {string}                                name
+   * @param {Object<string, number>|Array<string>}  locales
+   * @param {string}                                prePath
    *
    * @returns {IndexedFileSource}
    */
   constructor(name, locales, prePath) {
     this.name = name;
-    this.locales = locales;
+
+    if (!Array.isArray(locales) &&
+        Object.values(locales).some(ver => typeof ver !== "number")) {
+      throw new Error("All versions must be numbers");
+    }
+    this.locales = Array.isArray(locales) ?
+      new Map(locales.map(loc => [loc, 0])) :
+      new Map(Object.entries(locales));
     this.prePath = prePath;
     this.indexed = false;
 
     // The cache object stores information about the resources available
     // in the Source.
     //
     // It can take one of three states:
     //   * true - the resource is available but not fetched yet
@@ -298,17 +317,17 @@ class FileSource {
     this.cache = {};
   }
 
   getPath(locale, path) {
     return (this.prePath + path).replace(/\{locale\}/g, locale);
   }
 
   hasFile(locale, path) {
-    if (!this.locales.includes(locale)) {
+    if (!this.locales.has(locale)) {
       return false;
     }
 
     const fullPath = this.getPath(locale, path);
     if (!this.cache.hasOwnProperty(fullPath)) {
       return this.indexed ? false : undefined;
     }
     if (this.cache[fullPath] === false) {
@@ -316,17 +335,17 @@ class FileSource {
     }
     if (this.cache[fullPath].then) {
       return undefined;
     }
     return true;
   }
 
   fetchFile(locale, path) {
-    if (!this.locales.includes(locale)) {
+    if (!this.locales.has(locale)) {
       return Promise.reject(`The source has no resources for locale "${locale}"`);
     }
 
     const fullPath = this.getPath(locale, path);
 
     if (this.cache.hasOwnProperty(fullPath)) {
       if (this.cache[fullPath] === false) {
         return Promise.reject(`The source has no resources for path "${fullPath}"`);
@@ -355,20 +374,20 @@ class FileSource {
  * This is an extension of the FileSource which should be used
  * for sources that can provide the list of files available in the source.
  *
  * This allows for a faster lookup in cases where the source does not
  * contain most of the files that the app will request for (e.g. an addon).
  **/
 class IndexedFileSource extends FileSource {
   /**
-   * @param {string}         name
-   * @param {Array<string>}  locales
-   * @param {string}         prePath
-   * @param {Array<string>}  paths
+   * @param {string}                                name
+   * @param {Object<string, number>|Array<string>}  locales
+   * @param {string}                                prePath
+   * @param {Array<string>}                         paths
    *
    * @returns {IndexedFileSource}
    */
   constructor(name, locales, prePath, paths) {
     super(name, locales, prePath);
     this.indexed = true;
     for (const path of paths) {
       this.cache[path] = true;
--- a/intl/l10n/test/test_l10nregistry.js
+++ b/intl/l10n/test/test_l10nregistry.js
@@ -206,16 +206,58 @@ add_task(async function test_override() 
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
   L10nRegistry.ctxCache.clear();
 });
 
 /**
+ * This test checks if the correct order of contexts is used in case
+ * two sources provide the same locale but different versions.
+ */
+add_task(async function test_versions() {
+  let newerSource = new IndexedFileSource('langpack-pl', {'pl': 2}, '/langpack2/data/locales/{locale}/', [
+    '/langpack2/data/locales/pl/test.ftl'
+  ]);
+  L10nRegistry.registerSource(newerSource);
+
+  let olderSource = new IndexedFileSource('langpack-pl-old', {'pl': 1}, '/langpack1/data/locales/{locale}/', [
+    '/langpack1/data/locales/pl/test.ftl'
+  ]);
+  L10nRegistry.registerSource(olderSource);
+
+  fs = {
+    '/langpack1/data/locales/pl/test.ftl': 'key = old value',
+    '/langpack2/data/locales/pl/test.ftl': 'key = new value'
+  };
+
+  equal(L10nRegistry.sources.size, 2);
+
+  let ctxs = L10nRegistry.generateContexts(['pl'], ['test.ftl']);
+  let ctx0 = (await ctxs.next()).value;
+  equal(ctx0.locales[0], 'pl');
+  equal(ctx0.hasMessage('key'), true);
+  let msg0 = ctx0.getMessage('key');
+  equal(ctx0.format(msg0), 'new value');
+
+  let ctx1 = (await ctxs.next()).value;
+  equal(ctx1.locales[0], 'pl');
+  equal(ctx1.hasMessage('key'), true);
+  let msg1 = ctx1.getMessage('key');
+  equal(ctx1.format(msg1), 'old value');
+
+  equal((await ctxs.next()).done, true);
+
+  // cleanup
+  L10nRegistry.sources.clear();
+  L10nRegistry.ctxCache.clear();
+});
+
+/**
  * This test verifies that new contexts are returned
  * after source update.
  */
 add_task(async function test_updating() {
   let oneSource = new IndexedFileSource('langpack-pl', ['pl'], '/data/locales/{locale}/', [
     '/data/locales/pl/test.ftl',
   ]);
   L10nRegistry.registerSource(oneSource);
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -1722,17 +1722,20 @@ this.Langpack = class extends ExtensionD
       this.chromeRegistryHandle =
         aomStartup.registerChrome(manifestURI, this.startupData.chromeEntries);
     }
 
     const data = await this.parseManifest();
     this.langpackId = data.langpackId;
     this.l10nRegistrySources = data.l10nRegistrySources;
 
-    const languages = Object.keys(data.manifest.languages);
+    const languages = {};
+    for (const loc in data.manifest.languages) {
+      languages[loc] = parseInt(data.manifest.languages[loc].version);
+    }
     resourceProtocol.setSubstitution(this.langpackId, this.rootURI);
 
     for (const [sourceName, basePath] of Object.entries(this.l10nRegistrySources)) {
       L10nRegistry.registerSource(new FileSource(
         `${sourceName}-${this.langpackId}`,
         languages,
         `resource://${this.langpackId}/${basePath}localization/{locale}/`
       ));