Bug 1438608 - Improve error reporting of Fluent missing strings. r=stas
authorZibi Braniecki <zbraniecki@mozilla.com>
Tue, 06 Mar 2018 01:25:23 -0800
changeset 461733 8381ed38af109e758322070dd997e52cfab714fe
parent 461732 3ff3e950bcffbde279141b8fa37ccccdedaa04c4
child 461734 6bf29a26a10daad2756b8eb795b4b1a1c95a2da3
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstas
bugs1438608
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1438608 - Improve error reporting of Fluent missing strings. r=stas MozReview-Commit-ID: 5HOsUGtDi3V
intl/l10n/Localization.jsm
--- a/intl/l10n/Localization.jsm
+++ b/intl/l10n/Localization.jsm
@@ -19,16 +19,17 @@
 /* fluent@0.6.3 */
 
 /* eslint no-console: ["error", { allow: ["warn", "error"] }] */
 /* global console */
 
 const { XPCOMUtils } = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", {});
 const { L10nRegistry } = ChromeUtils.import("resource://gre/modules/L10nRegistry.jsm", {});
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm", {});
+const { AppConstants } = ChromeUtils.import("resource://gre/modules/AppConstants.jsm", {});
 
 /*
  * CachedIterable caches the elements yielded by an iterable.
  *
  * It can be used to iterate over an iterable many times without depleting the
  * iterable.
  */
 class CachedIterable {
@@ -86,34 +87,16 @@ class CachedIterable {
     const { seen, iterator } = this;
     if (seen.length === 0 || seen[seen.length - 1].done === false) {
       seen.push(iterator.next());
     }
   }
 }
 
 /**
- * Specialized version of an Error used to indicate errors that are result
- * of a problem during the localization process.
- *
- * We use them to identify the class of errors the require a fallback
- * mechanism to be triggered vs errors that should be reported, but
- * do not prevent the message from being used.
- *
- * An example of an L10nError is a missing entry.
- */
-class L10nError extends Error {
-  constructor(message) {
-    super();
-    this.name = "L10nError";
-    this.message = message;
-  }
-}
-
- /**
  * The default localization strategy for Gecko. It comabines locales
  * available in L10nRegistry, with locales requested by the user to
  * generate the iterator over MessageContexts.
  *
  * In the future, we may want to allow certain modules to override this
  * with a different negotitation strategy to allow for the module to
  * be localized into a different language - for example DevTools.
  */
@@ -151,27 +134,36 @@ class Localization {
    *
    * @param   {Array<Array>}          keys    - Translation keys to format.
    * @param   {Function}              method  - Formatting function.
    * @returns {Promise<Array<string|Object>>}
    * @private
    */
   async formatWithFallback(keys, method) {
     const translations = [];
+
     for await (let ctx of this.ctxs) {
       // This can operate on synchronous and asynchronous
       // contexts coming from the iterator.
       if (typeof ctx.then === "function") {
         ctx = await ctx;
       }
-      const errors = keysFromContext(method, ctx, keys, translations);
-      if (!errors) {
+      const missingIds = keysFromContext(method, ctx, keys, translations);
+
+      if (missingIds.size === 0) {
         break;
       }
+
+      if (AppConstants.NIGHTLY_BUILD) {
+        const locale = ctx.locales[0];
+        const ids = Array.from(missingIds).join(", ");
+        console.warn(`Missing translations in ${locale}: ${ids}`);
+      }
     }
+
     return translations;
   }
 
   /**
    * Format translations into {value, attrs} objects.
    *
    * The fallback logic is the same as in `formatValues` but the argument type
    * is stricter (an array of arrays) and it returns {value, attrs} objects
@@ -307,21 +299,16 @@ Localization.prototype.QueryInterface = 
  * @param   {string}         id
  * @param   {Object}         args
  * @returns {string}
  * @private
  */
 function valueFromContext(ctx, errors, id, args) {
   const msg = ctx.getMessage(id);
 
-  if (msg === undefined) {
-    errors.push(new L10nError(`Unknown entity: ${id}`));
-    return id;
-  }
-
   return ctx.format(msg, args, errors);
 }
 
 /**
  * Format all public values of a message into a { value, attrs } object.
  *
  * This function is passed as a method to `keysFromContext` and resolve
  * a single L10n Entity using provided `MessageContext`.
@@ -340,21 +327,16 @@ function valueFromContext(ctx, errors, i
  * @param   {String}         id
  * @param   {Object}         args
  * @returns {Object}
  * @private
  */
 function messageFromContext(ctx, errors, id, args) {
   const msg = ctx.getMessage(id);
 
-  if (msg === undefined) {
-    errors.push(new L10nError(`Unknown message: ${id}`));
-    return { value: id, attrs: null };
-  }
-
   const formatted = {
     value: ctx.format(msg, args, errors),
     attrs: null,
   };
 
   if (msg.attrs) {
     formatted.attrs = [];
     for (const name in msg.attrs) {
@@ -367,65 +349,59 @@ function messageFromContext(ctx, errors,
 
   return formatted;
 }
 
 /**
  * This function is an inner function for `Localization.formatWithFallback`.
  *
  * It takes a `MessageContext`, list of l10n-ids and a method to be used for
- * key resolution (either `valueFromContext` or `entityFromContext`) and
+ * key resolution (either `valueFromContext` or `messageFromContext`) and
  * optionally a value returned from `keysFromContext` executed against
  * another `MessageContext`.
  *
  * The idea here is that if the previous `MessageContext` did not resolve
  * all keys, we're calling this function with the next context to resolve
  * the remaining ones.
  *
  * In the function, we loop over `keys` and check if we have the `prev`
  * passed and if it has an error entry for the position we're in.
  *
  * If it doesn't, it means that we have a good translation for this key and
  * we return it. If it does, we'll try to resolve the key using the passed
  * `MessageContext`.
  *
- * In the end, we fill the translations array, and return if we
- * encountered at least one error.
+ * In the end, we fill the translations array, and return the Set with
+ * missing ids.
  *
  * See `Localization.formatWithFallback` for more info on how this is used.
  *
  * @param {Function}       method
  * @param {MessageContext} ctx
  * @param {Array<string>}  keys
  * @param {{Array<{value: string, attrs: Object}>}} translations
  *
- * @returns {Boolean}
+ * @returns {Set<string>}
  * @private
  */
 function keysFromContext(method, ctx, keys, translations) {
   const messageErrors = [];
-  let hasErrors = false;
+  const missingIds = new Set();
 
   keys.forEach((key, i) => {
     if (translations[i] !== undefined) {
       return;
     }
 
-    messageErrors.length = 0;
-    const translation = method(ctx, messageErrors, key[0], key[1]);
-
-    if (messageErrors.length === 0 ||
-        !messageErrors.some(e => e instanceof L10nError)) {
-      translations[i] = translation;
+    if (ctx.hasMessage(key[0])) {
+      messageErrors.length = 0;
+      translations[i] = method(ctx, messageErrors, key[0], key[1]);
+      // XXX: Report resolver errors
     } else {
-      hasErrors = true;
-    }
-
-    if (messageErrors.length) {
-      messageErrors.forEach(error => console.warn(error));
+      missingIds.add(key[0]);
     }
   });
 
-  return hasErrors;
+  return missingIds;
 }
 
 this.Localization = Localization;
 var EXPORTED_SYMBOLS = ["Localization"];