Bug 1455649 - DocumentL10n, part 3 - Update L10nRegistry to operate on FTLResources. draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Fri, 22 Jun 2018 12:03:24 -0700
changeset 815117 5246ee946c259a116dfb097b6a3dda110a0432f3
parent 815116 93448ee31634580275cfcd744219f0805fcebc44
child 815118 268829f20dde3f865af4589fd86c20325c83cdab
push id115447
push userbmo:gandalf@aviary.pl
push dateFri, 06 Jul 2018 20:45:09 +0000
bugs1455649
milestone63.0a1
Bug 1455649 - DocumentL10n, part 3 - Update L10nRegistry to operate on FTLResources. Switch to cache FTLResources per FileSource, and change the logic of context creation to return a context if at least one source for the permutation works. MozReview-Commit-ID: B9fxbkaU3oX
intl/l10n/L10nRegistry.jsm
intl/l10n/Localization.jsm
intl/l10n/test/test_l10nregistry.js
intl/l10n/test/test_localization.js
intl/l10n/test/test_pseudo.js
--- a/intl/l10n/L10nRegistry.jsm
+++ b/intl/l10n/L10nRegistry.jsm
@@ -74,34 +74,50 @@ XPCOMUtils.defineLazyGlobalGetters(this,
  *
  * 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) {
     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);
+      for await (let set of generateResourceSetsForLocale(locale, sourcesOrder, resourceIds)) {
+        let dataSet = await Promise.all(set);
+
+        if (dataSet.every(d => d === false)) {
+          continue;
+        }
+        const pseudoNameFromPref = Services.prefs.getStringPref("intl.l10n.pseudo", "");
+        const ctx = new MessageContext(locale, {
+          ...MSG_CONTEXT_OPTIONS,
+          transform: PSEUDO_STRATEGIES[pseudoNameFromPref],
+        });
+        for (const data of dataSet) {
+          if (data !== false) {
+            ctx.addDocument(data);
+          }
+        }
+        yield ctx;
+      }
     }
   },
 
   /**
    * Adds a new resource source to the L10nRegistry.
    *
    * @param {FileSource} source
    */
@@ -121,17 +137,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
    */
@@ -154,73 +169,47 @@ const L10nRegistry = {
         locales.add(locale);
       }
     }
     return Array.from(locales);
   },
 };
 
 /**
- * A helper function for generating unique context ID used for caching
- * MessageContexts.
- *
- * @param {String} locale
- * @param {Array} sourcesOrder
- * @param {Array} resourceIds
- * @returns {String}
- */
-function generateContextID(locale, sourcesOrder, resourceIds) {
-  const sources = sourcesOrder.join(",");
-  const ids = resourceIds.join(",");
-  return `${locale}|${sources}|${ids}`;
-}
-
-/**
  * This function generates an iterator over MessageContexts for a single locale
  * for a given list of resourceIds for all possible combinations of sources.
  *
  * This function is called recursively to generate all possible permutations
  * and uses the last, optional parameter, to pass the already resolved
  * 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;
-      }
+      yield generateResourceSet(locale, order, resourceIds);
     } 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),
@@ -341,45 +330,20 @@ const PSEUDO_STRATEGIES = {
  * 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) {
-  const ctxId = generateContextID(locale, sourcesOrder, resourceIds);
-  if (L10nRegistry.ctxCache.has(ctxId)) {
-    return L10nRegistry.ctxCache.get(ctxId);
-  }
-
-  const fetchPromises = resourceIds.map((resourceId, i) => {
+function generateResourceSet(locale, sourcesOrder, resourceIds) {
+  return resourceIds.map((resourceId, i) => {
     return L10nRegistry.sources.get(sourcesOrder[i]).fetchFile(locale, resourceId);
   });
-
-  const ctxPromise = Promise.all(fetchPromises).then(
-    dataSets => {
-      const pseudoNameFromPref = Services.prefs.getStringPref("intl.l10n.pseudo", "");
-      const ctx = new MessageContext(locale, {
-        ...MSG_CONTEXT_OPTIONS,
-        transform: PSEUDO_STRATEGIES[pseudoNameFromPref],
-      });
-      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
@@ -440,38 +404,35 @@ 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}"`);
-      }
-      if (this.cache[fullPath].then) {
-        return this.cache[fullPath];
+        return false;
       }
     } 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] = MessageContext.parseDocument(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.
@@ -505,17 +466,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/Localization.jsm
+++ b/intl/l10n/Localization.jsm
@@ -266,17 +266,16 @@ class Localization {
   observe(subject, topic, data) {
     switch (topic) {
       case "intl:app-locales-changed":
         this.onChange();
         break;
       case "nsPref:changed":
         switch (data) {
           case "intl.l10n.pseudo":
-            L10nRegistry.ctxCache.clear();
             this.onChange();
         }
         break;
       default:
         break;
     }
   }
 
--- 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;
 });
--- a/intl/l10n/test/test_localization.js
+++ b/intl/l10n/test/test_localization.js
@@ -38,17 +38,16 @@ add_task(async function test_methods_cal
   ], generateMessages);
 
   let values = await l10n.formatValues([{id: "key"}, {id: "key2"}]);
 
   equal(values[0], "[de] Value2");
   equal(values[1], "[en] Value3");
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
   Services.locale.setRequestedLocales(originalRequested);
 });
 
 add_task(async function test_builtins() {
   const { L10nRegistry, FileSource } =
     ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm", {});
 
@@ -85,17 +84,16 @@ key = { PLATFORM() ->
   ], generateMessages);
 
   let values = await l10n.formatValues([{id: "key"}]);
 
   ok(values[0].includes(
     `${ known_platforms[AppConstants.platform].toUpperCase() } Value`));
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
 });
 
 add_task(async function test_add_remove_resourceIds() {
   const { L10nRegistry, FileSource } =
     ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm", {});
 
   const fs = {
@@ -133,12 +131,11 @@ add_task(async function test_add_remove_
   l10n.removeResourceIds(["/browser/menu.ftl"]);
 
   values = await l10n.formatValues([{id: "key1"}, {id: "key2"}]);
 
   equal(values[0], undefined);
   equal(values[1], "Value2");
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalLoad;
   Services.locale.setRequestedLocales(originalRequested);
 });
--- a/intl/l10n/test/test_pseudo.js
+++ b/intl/l10n/test/test_pseudo.js
@@ -89,17 +89,16 @@ add_task(async function test_accented_wo
     let message = (await l10n.formatMessages([{id: "key"}]))[0];
 
     ok(message.value.includes("This is a single message"));
     ok(message.attributes[0].value.includes("This is a tooltip"));
     equal(message.attributes[1].value, "f");
   }
 
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalValues.load;
   Services.locale.setRequestedLocales(originalValues.requested);
 });
 
 /**
  * This test verifies that setting a bogus pseudo locale
  * strategy doesn't break anything.
  */
@@ -121,12 +120,11 @@ add_task(async function test_unavailable
 
     ok(message.value.includes("This is a single message"));
     ok(message.attributes[0].value.includes("This is a tooltip"));
     equal(message.attributes[1].value, "f");
   }
 
   Services.prefs.setStringPref("intl.l10n.pseudo", "");
   L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
   L10nRegistry.load = originalValues.load;
   Services.locale.setRequestedLocales(originalValues.requested);
 });