Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r?billm draft
authorKris Maglione <maglione.k@gmail.com>
Sun, 15 Nov 2015 16:54:41 -0800
changeset 308940 5392d1418e0f073fb3df133c985d7a22279054eb
parent 308939 51fa3e0d4f7bb2bf3457261091b1cb7a75e1255d
child 511228 8cfd51c2f58228d621805ad1085faf7b25905e63
push id7542
push usermaglione.k@gmail.com
push dateMon, 16 Nov 2015 00:59:26 +0000
reviewersbillm
bugs1221415
milestone45.0a1
Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r?billm
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionUtils.jsm
toolkit/components/extensions/test/mochitest/mochitest.ini
toolkit/components/extensions/test/mochitest/test_ext_i18n.html
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -61,16 +61,17 @@ ExtensionManagement.registerScript("chro
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 var {
   MessageBroker,
   Messenger,
   injectAPI,
   extend,
   flushJarCache,
+  instanceOf,
 } = ExtensionUtils;
 
 const LOGGER_ID_BASE = "addons.webextension.";
 
 var scriptScope = this;
 
 // This object loads the ext-*.js scripts that define the extension API.
 var Management = {
@@ -340,18 +341,17 @@ var GlobalManager = {
 this.ExtensionData = function(rootURI)
 {
   this.rootURI = rootURI;
 
   this.manifest = null;
   this.id = null;
   // Map(locale-name -> message-map)
   // Contains a key for each loaded locale, each of which is a
-  // JSON-compatible object with a property for each message
-  // in that locale.
+  // Map of message keys to their localized strings.
   this.localeMessages = new Map();
   this.selectedLocale = null;
   this._promiseLocales = null;
 
   this.errors = [];
 }
 
 ExtensionData.prototype = {
@@ -367,51 +367,44 @@ ExtensionData.prototype = {
 
   // Report an error about the extension's general packaging.
   packagingError(message) {
     this.errors.push(message);
     this.logger.error(`Loading extension '${this.id}': ${message}`);
   },
 
   // https://developer.chrome.com/extensions/i18n
-  localizeMessage(message, substitutions, locale = this.selectedLocale) {
-    let messages = {};
-    if (this.localeMessages.has(locale)) {
-      messages = this.localeMessages.get(locale);
-    }
+  localizeMessage(message, substitutions = [], locale = this.selectedLocale, defaultValue = "??") {
+    let locales = new Set([locale, this.defaultLocale]
+                          .filter(locale => this.localeMessages.has(locale)));
 
-    if (message in messages) {
-      let str = messages[message].message;
+    for (let locale of locales) {
+      let messages = this.localeMessages.get(locale);
+      if (messages.has(message)) {
+        let str = messages.get(message)
 
-      if (!substitutions) {
-        substitutions = [];
-      } else if (!Array.isArray(substitutions)) {
-        substitutions = [substitutions];
-      }
+        if (!Array.isArray(substitutions)) {
+          substitutions = [substitutions];
+        }
 
-      // https://developer.chrome.com/extensions/i18n-messages
-      // |str| may contain substrings of the form $1 or $PLACEHOLDER$.
-      // In the former case, we replace $n with substitutions[n - 1].
-      // In the latter case, we consult the placeholders array.
-      // The placeholder may itself use $n to refer to substitutions.
-      let replacer = (matched, name) => {
-        if (name.length == 1 && name[0] >= '1' && name[0] <= '9') {
-          return substitutions[parseInt(name) - 1];
-        } else {
-          let content = messages[message].placeholders[name].content;
-          if (content[0] == '$') {
-            return replacer(matched, content[1]);
+        let replacer = (matched, index, dollarSigns) => {
+          if (index) {
+            // This is not quite Chrome-compatible. Chrome consumes any number
+            // of digits following the $, but only accepts 9 substitutions. We
+            // accept any number of substitutions.
+            index = parseInt(index) - 1;
+            return index in substitutions ? substitutions[index] : "";
           } else {
-            return content;
+            // For any series of contiguous `$`s, the first is dropped, and
+            // the rest remain in the output string.
+            return dollarSigns;
           }
-        }
-      };
-      return str.replace(/\$([A-Za-z_@]+)\$/, replacer)
-                .replace(/\$([0-9]+)/, replacer)
-                .replace(/\$\$/, "$");
+        };
+        return str.replace(/\$(?:([1-9]\d*)|(\$+))/g, replacer);
+      }
     }
 
     // Check for certain pre-defined messages.
     if (message == "@@extension_id") {
       if ("uuid" in this) {
         // Per Chrome, this isn't available before an ID is guaranteed
         // to have been assigned, namely, in manifest files.
         // This should only be present in instances of the |Extension|
@@ -420,32 +413,32 @@ ExtensionData.prototype = {
       }
     } else if (message == "@@ui_locale") {
       return Locale.getLocale();
     } else if (message == "@@bidi_dir") {
       return "ltr"; // FIXME
     }
 
     Cu.reportError(`Unknown localization message ${message}`);
-    return "??";
+    return defaultValue;
   },
 
   // Localize a string, replacing all |__MSG_(.*)__| tokens with the
   // matching string from the current locale, as determined by
   // |this.selectedLocale|.
   //
   // This may not be called before calling either |initLocale| or
   // |initAllLocales|.
   localize(str, locale = this.selectedLocale) {
     if (!str) {
       return str;
     }
 
     return str.replace(/__MSG_([A-Za-z0-9@_]+?)__/g, (matched, message) => {
-      return this.localizeMessage(message, [], locale);
+      return this.localizeMessage(message, [], locale, matched);
     });
   },
 
   // If a "default_locale" is specified in that manifest, returns it
   // as a Gecko-compatible locale string. Otherwise, returns null.
   get defaultLocale() {
     if ("default_locale" in this.manifest) {
       return this.normalizeLocaleCode(this.manifest.default_locale);
@@ -560,26 +553,76 @@ ExtensionData.prototype = {
       if (typeof this.id != "string") {
         this.manifestError("Missing required `applications.gecko.id` property");
       }
 
       return manifest;
     });
   },
 
+  // Validates the contents of a locale JSON file, normalizes the
+  // messages into a Map of message key -> localized string pairs.
+  processLocale(locale, messages) {
+    let result = new Map();
+
+    // Chrome does not document the semantics of its localization
+    // system very well. It handles replacements by pre-processing
+    // messages, replacing |$[a-zA-Z0-9@_]+$| tokens with the value of their
+    // replacements. Later, it processes the resulting string for
+    // |$[0-9]| replacements.
+    //
+    // Again, it does not document this, but it accepts any number
+    // of sequential |$|s, and replaces them with that number minus
+    // 1. It also accepts |$| followed by any number of sequential
+    // digits, but refuses to process a localized string which
+    // provides more than 9 substitutions.
+    if (!instanceOf(messages, "Object")) {
+      this.packagingError(`Invalid locale data for ${locale}`);
+      return result;
+    }
+
+    for (let key of Object.keys(messages)) {
+      let msg = messages[key];
+
+      if (!instanceOf(msg, "Object") || typeof msg.message != "string") {
+        this.packagingError(`Invalid locale message data for ${locale}, message ${JSON.stringify(key)}`);
+        continue;
+      }
+
+      let { replacements } = msg;
+      if (!instanceOf(replacements, "Object")) {
+        replacements = {};
+      }
+
+      let replacer = (match, name) => {
+        let replacement = replacements[name];
+        if (instanceOf(replacement, "Object") && "content" in replacement) {
+          return replacement.content;
+        }
+        return "";
+      };
+
+      let value = msg.message.replace(/\$([A-Za-z0-9@_]+)\$/g, replacer);
+      result.set(key, value);
+    }
+
+    return result;
+  },
+
   // Reads the locale file for the given Gecko-compatible locale code, and
   // stores its parsed contents in |this.localeMessages.get(locale)|.
   readLocaleFile: Task.async(function* (locale) {
     let locales = yield this.promiseLocales();
     let dir = locales.get(locale);
     let file = `_locales/${dir}/messages.json`;
 
-    let messages = {};
+    let messages = new Map;
     try {
       messages = yield this.readJSON(file);
+      messages = this.processLocale(locale, messages);
     } catch (e) {
       this.packagingError(`Loading locale file ${file}: ${e}`);
     }
 
     this.localeMessages.set(locale, messages);
     return messages;
   }),
 
@@ -772,17 +815,17 @@ this.Extension.generate = function(id, d
   if (data.background) {
     let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
     let bgScript = uuidGenerator.generateUUID().number + ".js";
 
     provide(manifest, ["background", "scripts"], [bgScript], true);
     files[bgScript] = data.background;
   }
 
-  provide(files, ["manifest.json"], JSON.stringify(manifest));
+  provide(files, ["manifest.json"], manifest);
 
   let ZipWriter = Components.Constructor("@mozilla.org/zipwriter;1", "nsIZipWriter");
   let zipW = new ZipWriter();
 
   let file = FileUtils.getFile("TmpD", ["generated-extension.xpi"]);
   file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
 
   const MODE_WRONLY = 0x02;
@@ -802,16 +845,18 @@ this.Extension.generate = function(id, d
       }
     }
   }
 
   for (let filename in files) {
     let script = files[filename];
     if (typeof(script) == "function") {
       script = "(" + script.toString() + ")()";
+    } else if (typeof script == "object") {
+      script = JSON.stringify(script);
     }
 
     let stream = Cc["@mozilla.org/io/string-input-stream;1"].createInstance(Ci.nsIStringInputStream);
     stream.data = script;
 
     generateFile(filename);
     zipW.addEntryStream(filename, time, 0, stream, false);
   }
@@ -932,28 +977,41 @@ Extension.prototype = extend(Object.crea
   forgetOnClose(obj) {
     this.onShutdown.delete(obj);
   },
 
   // Reads the locale file for the given Gecko-compatible locale code, or if
   // no locale is given, the available locale closest to the UI locale.
   // Sets the currently selected locale on success.
   initLocale: Task.async(function* (locale = undefined) {
+    let promises = [];
+
     if (locale === undefined) {
       let locales = yield this.promiseLocales();
 
-      let localeList = Object.keys(locales).map(locale => {
+
+      let localeList = Array.from(locales.keys(), locale => {
         return { name: locale, locales: [locale] };
       });
 
       let match = Locale.findClosestLocale(localeList);
-      locale = match ? match.name : this.defaultLocale;
+      if (match) {
+        locale = match.name;
+        if (locale != this.defaultLocale) {
+          // Load the default locale for fallback strings.
+          promises.push(this.readLocaleFile(this.defaultLocale));
+        }
+      } else {
+        locale = this.defaultLocale;
+      }
     }
 
-    return ExtensionData.prototype.initLocale.call(this, locale);
+    promises.push(ExtensionData.prototype.initLocale.call(this, locale));
+
+    return Promise.all(promises).then(results => results.pop());
   }),
 
   startup() {
     try {
       ExtensionManagement.startupExtension(this.uuid, this.addonData.resourceURI, this);
     } catch (e) {
       return Promise.reject(e);
     }
@@ -966,17 +1024,17 @@ Extension.prototype = extend(Object.crea
       }
 
       GlobalManager.init(this);
 
       Management.emit("startup", this);
 
       return this.runManifest(this.manifest);
     }).catch(e => {
-      dump(`Extension error: ${e} ${e.filename}:${e.lineNumber}\n`);
+      dump(`Extension error: ${e} ${e.filename || e.fileName}:${e.lineNumber}\n`);
       Cu.reportError(e);
       throw e;
     });
   },
 
   cleanupGeneratedFile() {
     if (!this.cleanupFile) {
       return;
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -59,16 +59,22 @@ function runSafe(context, f, ...args)
     args = Cu.cloneInto(args, context.cloneScope);
   } catch (e) {
     Cu.reportError(e);
     dump(`runSafe failure: cloning into ${context.cloneScope}: ${e}\n\n${Error().stack}`);
   }
   return runSafeWithoutClone(f, ...args);
 }
 
+// Return true if the given value is an instance of the given
+// native type.
+function instanceOf(value, type) {
+  return {}.toString.call(value) == `[object ${type}]`;
+}
+
 // Extend the object |obj| with the property descriptors of each object in
 // |args|.
 function extend(obj, ...args) {
   for (let arg of args) {
     let props = [...Object.getOwnPropertyNames(arg),
                  ...Object.getOwnPropertySymbols(arg)];
     for (let prop of props) {
       let descriptor = Object.getOwnPropertyDescriptor(arg, prop);
@@ -629,9 +635,10 @@ this.ExtensionUtils = {
   EventManager,
   SingletonEventManager,
   ignoreEvent,
   injectAPI,
   MessageBroker,
   Messenger,
   extend,
   flushJarCache,
+  instanceOf,
 };
--- a/toolkit/components/extensions/test/mochitest/mochitest.ini
+++ b/toolkit/components/extensions/test/mochitest/mochitest.ini
@@ -32,8 +32,9 @@ support-files =
 [test_ext_storage.html]
 [test_ext_background_runtime_connect_params.html]
 [test_ext_cookies.html]
 [test_ext_bookmarks.html]
 [test_ext_alarms.html]
 [test_ext_background_window_properties.html]
 [test_ext_jsversion.html]
 skip-if = e10s # Uses a console monitor which doesn't work from a content process. The code being tested doesn't run in a tab content process in any case.
+[test_ext_i18n.html]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/mochitest/test_ext_i18n.html
@@ -0,0 +1,121 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <title>Test for WebExtension localization APIs</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/ExtensionTestUtils.js"></script>
+  <script type="text/javascript" src="head.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+
+<script type="application/javascript;version=1.8">
+
+add_task(function* test_i18n() {
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      "default_locale": "jp",
+    },
+
+    files: {
+      "_locales/en_US/messages.json": {
+        "foo": {
+          "message": "Foo.",
+          "description": "foo",
+        },
+
+        "basic_substitutions": {
+          "message": "'$0' '$14' '$1' '$5' '$$$$$' '$$'.",
+          "description": "foo",
+        },
+
+        "named_replacement_substitutions": {
+          "message": "$foo$\n$2\n$bad name$\n$bad_value$\n$bad_content_value$\n$foo",
+          "description": "foo",
+          "replacements": {
+            "foo": {
+              "content": "_foo_ $1 _bar_",
+              "description": "foo",
+            },
+
+            "bad name": {
+              "content": "Nope.",
+              "description": "bad name",
+            },
+
+            "bad_value": "Nope.",
+
+            "bad_content_value": {
+              "content": ["Accepted, but shouldn't break."],
+              "description": "bad value",
+            },
+          },
+        },
+
+        "broken_replacements": {
+          "message": "$broken$",
+          "description": "broken replacements",
+          "replacements": "foo.",
+        },
+      },
+
+      "_locales/jp/messages.json": {
+        "foo": {
+          "message": "(foo)",
+          "description": "foo",
+        },
+
+        "bar": {
+          "message": "(bar)",
+          "description": "bar",
+        },
+      },
+    },
+
+    background: "new " + function() {
+      let _ = browser.i18n.getMessage.bind(browser.i18n);
+
+      browser.test.assertEq("Foo.", _("foo"), "Simple message in selected locale.");
+
+      browser.test.assertEq("(bar)", _("bar"), "Simple message fallback in default locale.");
+
+      let substitutions = [];
+      substitutions[4] = "5";
+      substitutions[13] = "14";
+
+      browser.test.assertEq("'$0' '14' '' '5' '$$$$' '$'.",
+                            _("basic_substitutions", substitutions),
+                            "Basic numeric substitutions");
+
+      browser.test.assertEq("'$0' '' 'just a string' '' '$$$$' '$'.",
+                            _("basic_substitutions", "just a string"),
+                            "Basic numeric substitutions, with non-array value");
+
+      let values = _("named_replacement_substitutions", ["(subst $1 $2)", "(2 $1 $2)"]).split("\n");
+
+      browser.test.assertEq("_foo_ (subst $1 $2) _bar_", values[0], "Named and numeric substitution");
+
+      browser.test.assertEq("(2 $1 $2)", values[1], "Numeric substitution amid named substitutions");
+
+      browser.test.assertEq("$bad name$", values[2], "Named substitution with invalid key");
+
+      browser.test.assertEq("", values[3], "Named substitution with an invalid value");
+
+      browser.test.assertEq("Accepted, but shouldn't break.", values[4], "Named substitution with a strange content value");
+
+      browser.test.assertEq("$foo", values[5], "Non-substitution token that should be ignored");
+
+      browser.test.notifyPass("l10n");
+    },
+  });
+
+  yield extension.startup();
+  yield extension.awaitFinish("l10n");
+  yield extension.unload();
+});
+
+</script>
+
+</body>
+</html>