Bug 1462839 - Switch L10nRegistry to cache FTLResource objects, prefetch resources and return incomplete contexts. draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Mon, 04 Jun 2018 15:59:15 -0700
changeset 804490 54df2827edbdccb775c7fa59623c9a9b64c1df4a
parent 802528 42880a726964a0bd66e2f636931e8322eae86ef7
push id112384
push userbmo:gandalf@aviary.pl
push dateWed, 06 Jun 2018 00:13:47 +0000
bugs1462839
milestone62.0a1
Bug 1462839 - Switch L10nRegistry to cache FTLResource objects, prefetch resources and return incomplete contexts. MozReview-Commit-ID: GYQ0YfWkrLL
intl/l10n/L10nRegistry.jsm
intl/l10n/MessageContext.jsm
intl/l10n/l10n.js
intl/l10n/test/test_l10nregistry.js
--- a/intl/l10n/L10nRegistry.jsm
+++ b/intl/l10n/L10nRegistry.jsm
@@ -1,11 +1,11 @@
 const { AppConstants } = ChromeUtils.import("resource://gre/modules/AppConstants.jsm", {});
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm", {});
-const { MessageContext } = ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
+const { MessageContext, FTLResource } = ChromeUtils.import("resource://gre/modules/MessageContext.jsm", {});
 Cu.importGlobalProperties(["fetch"]);
 
 /**
  * L10nRegistry is a localization resource management system for Gecko.
  *
  * It manages the list of resource sources provided with the app and allows
  * for additional sources to be added and updated.
  *
@@ -73,34 +73,53 @@ Cu.importGlobalProperties(["fetch"]);
  *
  * If during the life-cycle of the app a new source is added, the generator can be called again
  * and will produce a new set of permutations placing the language pack provided resources
  * at the top.
  */
 
 const L10nRegistry = {
   sources: new Map(),
-  ctxCache: new Map(),
   bootstrap: null,
 
   /**
    * Based on the list of requested languages and resource Ids,
    * this function returns an lazy iterator over message context permutations.
    *
    * @param {Array} requestedLangs
    * @param {Array} resourceIds
    * @returns {AsyncIterator<MessageContext>}
    */
   async* generateContexts(requestedLangs, resourceIds) {
+    for await (let [locale, set] of this.prefetch(requestedLangs, resourceIds, null)) {
+      let dataSet = await Promise.all(set);
+
+      if (dataSet.every(d => d === false)) {
+        continue;
+      }
+      const ctx = new MessageContext(locale, MSG_CONTEXT_OPTIONS);
+      for (const data of dataSet) {
+        if (data !== false) {
+          ctx.loadResource(data);
+        }
+      }
+      yield ctx;
+    }
+  },
+
+  async* prefetch(requestedLangs, resourceIds, limit = 2) {
     if (this.bootstrap !== null) {
       await this.bootstrap;
     }
     const sourcesOrder = Array.from(this.sources.keys()).reverse();
     for (const locale of requestedLangs) {
-      yield * generateContextsForLocale(locale, sourcesOrder, resourceIds);
+      if (limit !== null && --limit === 0) {
+        return;
+      }
+      yield * generateResourceSetsForLocale(locale, sourcesOrder, resourceIds);
     }
   },
 
   /**
    * Adds a new resource source to the L10nRegistry.
    *
    * @param {FileSource} source
    */
@@ -120,17 +139,16 @@ const L10nRegistry = {
    *
    * @param {FileSource} source
    */
   updateSource(source) {
     if (!this.sources.has(source.name)) {
       throw new Error(`Source with name "${source.name}" is not registered.`);
     }
     this.sources.set(source.name, source);
-    this.ctxCache.clear();
     Services.locale.setAvailableLocales(this.getAvailableLocales());
   },
 
   /**
    * Removes a source from the L10nRegistry.
    *
    * @param {String} sourceId
    */
@@ -181,45 +199,35 @@ function generateContextID(locale, sourc
  * sources order.
  *
  * @param {String} locale
  * @param {Array} sourcesOrder
  * @param {Array} resourceIds
  * @param {Array} [resolvedOrder]
  * @returns {AsyncIterator<MessageContext>}
  */
-async function* generateContextsForLocale(locale, sourcesOrder, resourceIds, resolvedOrder = []) {
+function* generateResourceSetsForLocale(locale, sourcesOrder, resourceIds, resolvedOrder = []) {
   const resolvedLength = resolvedOrder.length;
   const resourcesLength = resourceIds.length;
 
   // Inside that loop we have a list of resources and the sources for them, like this:
   //   ['test.ftl', 'menu.ftl', 'foo.ftl']
   //   ['app', 'platform', 'app']
   for (const sourceName of sourcesOrder) {
     const order = resolvedOrder.concat(sourceName);
 
-    // We bail only if the hasFile returns a strict false here,
-    // because for FileSource it may also return undefined, which means
-    // that we simply don't know if the source contains the file and we'll
-    // have to perform the I/O to learn.
-    if (L10nRegistry.sources.get(sourceName).hasFile(locale, resourceIds[resolvedOrder.length]) === false) {
-      continue;
-    }
-
     // If the number of resolved sources equals the number of resources,
     // create the right context and return it if it loads.
     if (resolvedLength + 1 === resourcesLength) {
-      const ctx = await generateContext(locale, order, resourceIds);
-      if (ctx !== null) {
-        yield ctx;
-      }
+      const set = generateResourceSet(locale, order, resourceIds);
+      yield [locale, set];
     } else if (resolvedLength < resourcesLength) {
       // otherwise recursively load another generator that walks over the
       // partially resolved list of sources.
-      yield * generateContextsForLocale(locale, sourcesOrder, resourceIds, order);
+      yield * generateResourceSetsForLocale(locale, sourcesOrder, resourceIds, order);
     }
   }
 }
 
 const MSG_CONTEXT_OPTIONS = {
   // Temporarily disable bidi isolation due to Microsoft not supporting FSI/PDI.
   // See bug 1439018 for details.
   useIsolating: Services.prefs.getBoolPref("intl.l10n.enable-bidi-marks", false),
@@ -253,41 +261,23 @@ const MSG_CONTEXT_OPTIONS = {
  * This allows the caller to be an async generator without using
  * try/catch clauses.
  *
  * @param {String} locale
  * @param {Array} sourcesOrder
  * @param {Array} resourceIds
  * @returns {Promise<MessageContext>}
  */
-function generateContext(locale, sourcesOrder, resourceIds) {
+function generateResourceSet(locale, sourcesOrder, resourceIds) {
   const ctxId = generateContextID(locale, sourcesOrder, resourceIds);
-  if (L10nRegistry.ctxCache.has(ctxId)) {
-    return L10nRegistry.ctxCache.get(ctxId);
-  }
 
-  const fetchPromises = resourceIds.map((resourceId, i) => {
+  return resourceIds.map((resourceId, i) => {
     return L10nRegistry.sources.get(sourcesOrder[i]).fetchFile(locale, resourceId);
   });
 
-  const ctxPromise = Promise.all(fetchPromises).then(
-    dataSets => {
-      const ctx = new MessageContext(locale, MSG_CONTEXT_OPTIONS);
-      for (const data of dataSets) {
-        if (data === null) {
-          return null;
-        }
-        ctx.addMessages(data);
-      }
-      return ctx;
-    },
-    () => null
-  );
-  L10nRegistry.ctxCache.set(ctxId, ctxPromise);
-  return ctxPromise;
 }
 
 /**
  * This is a basic Source for L10nRegistry.
  * 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
@@ -348,38 +338,38 @@ class FileSource {
     if (this.cache[fullPath].then) {
       return undefined;
     }
     return true;
   }
 
   fetchFile(locale, path) {
     if (!this.locales.includes(locale)) {
-      return Promise.reject(`The source has no resources for locale "${locale}"`);
+      return false;
     }
 
     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}"`);
+        return false;
       }
       if (this.cache[fullPath].then) {
         return this.cache[fullPath];
       }
     } else if (this.indexed) {
-        return Promise.reject(`The source has no resources for path "${fullPath}"`);
-      }
+      return false;
+    }
     return this.cache[fullPath] = L10nRegistry.load(fullPath).then(
       data => {
-        return this.cache[fullPath] = data;
+        return this.cache[fullPath] = new FTLResource(data);
       },
       err => {
         this.cache[fullPath] = false;
-        return Promise.reject(err);
+        return false;
       }
     );
   }
 }
 
 /**
  * This is an extension of the FileSource which should be used
  * for sources that can provide the list of files available in the source.
@@ -413,17 +403,17 @@ class IndexedFileSource extends FileSour
  *
  * @param {string} url
  *
  * @returns {Promise<string>}
  */
 L10nRegistry.load = function(url) {
   return fetch(url).then(response => {
     if (!response.ok) {
-      return Promise.reject(response.statusText);
+      return false;
     }
     return response.text();
   });
 };
 
 this.L10nRegistry = L10nRegistry;
 this.FileSource = FileSource;
 this.IndexedFileSource = IndexedFileSource;
--- a/intl/l10n/MessageContext.jsm
+++ b/intl/l10n/MessageContext.jsm
@@ -21,16 +21,22 @@
 /*  eslint no-magic-numbers: [0]  */
 
 const MAX_PLACEABLES = 100;
 
 const entryIdentifierRe = /-?[a-zA-Z][a-zA-Z0-9_-]*/y;
 const identifierRe = /[a-zA-Z][a-zA-Z0-9_-]*/y;
 const functionIdentifierRe = /^[A-Z][A-Z_?-]*$/;
 
+class FTLResource {
+  constructor(source) {
+    const [entries, errors] = parse(source);
+    this._entries = entries;
+  }
+}
 /**
  * The `Parser` class is responsible for parsing FTL resources.
  *
  * It's only public method is `getResource(source)` which takes an FTL string
  * and returns a two element Array with an Object of entries generated from the
  * source as the first element and an array of SyntaxError objects as the
  * second.
  *
@@ -1770,16 +1776,27 @@ class MessageContext {
    * Parsed entities should be formatted with the `format` method in case they
    * contain logic (references, select expressions etc.).
    *
    * @param   {string} source - Text resource with translations.
    * @returns {Array<Error>}
    */
   addMessages(source) {
     const [entries, errors] = parse(source);
+
+    this.loadEntries(entries, errors);
+
+    return errors;
+  }
+
+  loadResource(res) {
+    this.loadEntries(res._entries);
+  }
+
+  loadEntries(entries, errors = []) {
     for (const id in entries) {
       if (id.startsWith("-")) {
         // Identifiers starting with a dash (-) define terms. Terms are private
         // and cannot be retrieved from MessageContext.
         if (this._terms.has(id)) {
           errors.push(`Attempt to override an existing term: "${id}"`);
           continue;
         }
@@ -1787,17 +1804,16 @@ class MessageContext {
       } else {
         if (this._messages.has(id)) {
           errors.push(`Attempt to override an existing message: "${id}"`);
           continue;
         }
         this._messages.set(id, entries[id]);
       }
     }
-
     return errors;
   }
 
   /**
    * Format a message to a string or null.
    *
    * Format a raw `message` from the context into a string (or a null if it has
    * a null value).  `args` will be used to resolve references to external
@@ -1854,9 +1870,10 @@ class MessageContext {
       this._intls.set(ctor, cache);
     }
 
     return cache[id];
   }
 }
 
 this.MessageContext = MessageContext;
-var EXPORTED_SYMBOLS = ["MessageContext"];
+this.FTLResource = FTLResource;
+var EXPORTED_SYMBOLS = ["MessageContext", "FTLResource"];
--- a/intl/l10n/l10n.js
+++ b/intl/l10n/l10n.js
@@ -1,11 +1,13 @@
 {
   const { DOMLocalization } =
     ChromeUtils.import("resource://gre/modules/DOMLocalization.jsm", {});
+  const { L10nRegistry } = ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm", {});
+  const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm", {});
 
   /**
    * Polyfill for document.ready polyfill.
    * See: https://github.com/whatwg/html/issues/127 for details.
    *
    * @returns {Promise}
    */
   function documentReady() {
@@ -42,16 +44,17 @@
     );
   }
 
   const resourceIds = getResourceLinks(document.head || document);
 
   document.l10n = new DOMLocalization(window, resourceIds);
 
   // Trigger the first two contexts to be loaded eagerly.
-  document.l10n.ctxs.touchNext(2);
+  const appLocales = Services.locale.getAppLocalesAsBCP47();
+  L10nRegistry.prefetch(appLocales, resourceIds, 2);
 
   document.l10n.ready = documentReady().then(() => {
     document.l10n.registerObservers();
     document.l10n.connectRoot(document.documentElement);
     return document.l10n.translateRoots();
   });
 }
--- a/intl/l10n/test/test_l10nregistry.js
+++ b/intl/l10n/test/test_l10nregistry.js
@@ -35,34 +35,32 @@ add_task(async function test_empty_resou
   const ctxs = L10nRegistry.generateContexts(["en-US"], []);
 
   const done = (await ctxs.next()).done;
 
   equal(done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * Test that passing empty sources list works.
  */
 add_task(async function test_empty_sources() {
   fs = {};
 
   const ctxs = L10nRegistry.generateContexts(["en-US"], []);
 
   const done = (await ctxs.next()).done;
 
   equal(done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test tests generation of a proper context for a single
  * source scenario
  */
 add_task(async function test_methods_calling() {
   fs = {
@@ -75,17 +73,16 @@ add_task(async function test_methods_cal
   const ctxs = L10nRegistry.generateContexts(["en-US"], ["/browser/menu.ftl"]);
 
   const ctx = (await ctxs.next()).value;
 
   equal(ctx.hasMessage("key"), true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that the public methods return expected values
  * for the single source scenario
  */
 add_task(async function test_has_one_source() {
   let oneSource = new FileSource("app", ["en-US"], "./app/data/locales/{locale}/");
@@ -113,17 +110,16 @@ add_task(async function test_has_one_sou
   // returns no contexts for missing locale
 
   ctxs = L10nRegistry.generateContexts(["pl"], ["test.ftl"]);
 
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that public methods return expected values
  * for the dual source scenario.
  */
 add_task(async function test_has_two_sources() {
   let oneSource = new FileSource("platform", ["en-US"], "./platform/data/locales/{locale}/");
@@ -170,17 +166,16 @@ add_task(async function test_has_two_sou
   equal(ctx1.hasMessage("key"), true);
   let msg1 = ctx1.getMessage("key");
   equal(ctx1.format(msg1), "platform value");
 
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that behavior specific to the IndexedFileSource
  * works correctly.
  *
  * In particular it tests that IndexedFileSource correctly returns
  * missing files as `false` instead of `undefined`.
@@ -198,17 +193,16 @@ add_task(async function test_indexed() {
   equal(L10nRegistry.sources.has("langpack-pl"), true);
 
   equal(oneSource.getPath("pl", "test.ftl"), "/data/locales/pl/test.ftl");
   equal(oneSource.hasFile("pl", "test.ftl"), true);
   equal(oneSource.hasFile("pl", "missing.ftl"), false);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test checks if the correct order of contexts is used for
  * scenarios where a new file source is added on top of the default one.
  */
 add_task(async function test_override() {
   let fileSource = new FileSource("app", ["pl"], "/app/data/locales/{locale}/");
@@ -239,17 +233,16 @@ add_task(async function test_override() 
   equal(ctx1.hasMessage("key"), true);
   let msg1 = ctx1.getMessage("key");
   equal(ctx1.format(msg1), "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}/", [
@@ -277,17 +270,16 @@ add_task(async function test_updating() 
   equal(L10nRegistry.sources.size, 1);
   ctxs = L10nRegistry.generateContexts(["pl"], ["test.ftl"]);
   ctx0 = (await ctxs.next()).value;
   msg0 = ctx0.getMessage("key");
   equal(ctx0.format(msg0), "new value");
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that generated contexts return correct values
  * after sources are being removed.
  */
 add_task(async function test_removing() {
   let fileSource = new FileSource("app", ["pl"], "/app/data/locales/{locale}/");
@@ -343,17 +335,16 @@ add_task(async function test_removing() 
 
   equal(L10nRegistry.sources.size, 0);
 
   ctxs = L10nRegistry.generateContexts(["pl"], ["test.ftl"]);
   equal((await ctxs.next()).done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that the logic works correctly when there's a missing
  * file in the FileSource scenario.
  */
 add_task(async function test_missing_file() {
   let oneSource = new FileSource("app", ["en-US"], "./app/data/locales/{locale}/");
@@ -373,25 +364,26 @@ add_task(async function test_missing_fil
   equal(L10nRegistry.sources.size, 2);
   equal(L10nRegistry.sources.has("app"), true);
   equal(L10nRegistry.sources.has("platform"), true);
 
 
   // returns a single context
 
   let ctxs = L10nRegistry.generateContexts(["en-US"], ["test.ftl", "test2.ftl"]);
-  (await ctxs.next());
-  (await ctxs.next());
+  (await ctxs.next()); // [platform, platform] - both present
+  (await ctxs.next()); // [platform, app] - second missing
+  (await ctxs.next()); // [app, platform] - both present
+  (await ctxs.next()); // [app, app] - second missing
 
   equal((await ctxs.next()).done, true);
 
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that each file is that all files requested
  * by a single context are fetched at the same time, even
  * if one I/O is slow.
  */
 add_task(async function test_parallel_io() {
@@ -442,11 +434,10 @@ add_task(async function test_parallel_io
   equal((await ctxs.next()).done, true);
 
   // When requested again, the cache should make the load operation not
   // increase the fetchedIndex count
   L10nRegistry.generateContexts(["en-US"], ["test.ftl", "test2.ftl", "slow-file.ftl"]);
 
   // cleanup
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
 });