Backed out changeset 97dbcd99280d (bug 1394977) for xpcshell failures in test_localization.js
authorPhil Ringnalda <philringnalda@gmail.com>
Fri, 08 Sep 2017 22:21:00 -0700
changeset 429340 b8254b550772912407a04b3c771e42b688d338e9
parent 429339 89f84623ff979916be058808e84713b1b423e180
child 429341 e0d88cd771d6791be4eaed4ada9315f4b26e2ad1
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1394977
milestone57.0a1
backs out97dbcd99280dad5c762e10e596e39f286772348e
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
Backed out changeset 97dbcd99280d (bug 1394977) for xpcshell failures in test_localization.js MozReview-Commit-ID: 1ZyUZB7cigl
intl/l10n/L10nRegistry.jsm
intl/l10n/test/test_l10nregistry.js
--- a/intl/l10n/L10nRegistry.jsm
+++ b/intl/l10n/L10nRegistry.jsm
@@ -31,17 +31,17 @@ Components.utils.importGlobalProperties(
  *     ]
  *
  * If the user will request:
  *   L10nRegistry.generateContexts(['de', 'en-US'], [
  *     '/browser/menu.ftl',
  *     '/platform/toolkit.ftl'
  *   ]);
  *
- * the generator will return an async iterator over the following contexts:
+ * the generator will return an iterator over the following contexts:
  *
  *   {
  *     locale: 'de',
  *     resources: [
  *       ['app', '/browser/menu.ftl'],
  *       ['app', '/platform/toolkit.ftl'],
  *     ]
  *   },
@@ -80,19 +80,19 @@ const L10nRegistry = {
   ctxCache: new Map(),
 
   /**
    * 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>}
+   * @returns {Iterator<MessageContext>}
    */
-  async * generateContexts(requestedLangs, resourceIds) {
+  * generateContexts(requestedLangs, resourceIds) {
     const sourcesOrder = Array.from(this.sources.keys()).reverse();
     for (const locale of requestedLangs) {
       yield * generateContextsForLocale(locale, sourcesOrder, resourceIds);
     }
   },
 
   /**
    * Adds a new resource source to the L10nRegistry.
@@ -174,19 +174,19 @@ function generateContextID(locale, sourc
  * 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>}
+ * @returns {Iterator<MessageContext>}
  */
-async function* generateContextsForLocale(locale, sourcesOrder, resourceIds, resolvedOrder = []) {
+function* generateContextsForLocale(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);
@@ -197,202 +197,150 @@ async function* generateContextsForLocal
     // 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 generateContext(locale, order, resourceIds);
     } else {
       // otherwise recursively load another generator that walks over the
       // partially resolved list of sources.
       yield * generateContextsForLocale(locale, sourcesOrder, resourceIds, order);
     }
   }
 }
 
 /**
  * Generates a single MessageContext by loading all resources
  * from the listed sources for a given locale.
  *
- * The function casts all error cases into a Promise that resolves with
- * value `null`.
- * 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) {
+async function generateContext(locale, sourcesOrder, resourceIds) {
   const ctxId = generateContextID(locale, sourcesOrder, resourceIds);
-  if (L10nRegistry.ctxCache.has(ctxId)) {
-    return L10nRegistry.ctxCache.get(ctxId);
+  if (!L10nRegistry.ctxCache.has(ctxId)) {
+    const ctx = new MessageContext(locale);
+    for (let i = 0; i < resourceIds.length; i++) {
+      const data = await L10nRegistry.sources.get(sourcesOrder[i]).fetchFile(locale, resourceIds[i]);
+      if (data === null) {
+        return false;
+      }
+      ctx.addMessages(data);
+    }
+    L10nRegistry.ctxCache.set(ctxId, ctx);
   }
-
-  const fetchPromises = 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);
-      for (const data of dataSets) {
-        if (data === null) {
-          return null;
-        }
-        ctx.addMessages(data);
-      }
-      return ctx;
-    },
-    () => null
-  );
-  L10nRegistry.ctxCache.set(ctxId, ctxPromise);
-  return ctxPromise;
+  return L10nRegistry.ctxCache.get(ctxId);
 }
 
 /**
  * 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
  * come from the cache.
  **/
 class FileSource {
-  /**
-   * @param {string}         name
-   * @param {Array<string>}  locales
-   * @param {string}         prePath
-   *
-   * @returns {IndexedFileSource}
-   */
   constructor(name, locales, prePath) {
     this.name = name;
     this.locales = 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
-    //   * false - the resource is not available
-    //   * Promise - the resource has been fetched
-    //
-    // If the cache has no entry for a given path, that means that there
-    // is no information available about whether the resource is available.
-    //
-    // If the `indexed` property is set to `true` it will be treated as the
-    // resource not being available. Otherwise, the resource may be
-    // available and we do not have any information about it yet.
     this.cache = {};
   }
 
   getPath(locale, path) {
     return (this.prePath + path).replace(/\{locale\}/g, locale);
   }
 
   hasFile(locale, path) {
     if (!this.locales.includes(locale)) {
       return false;
     }
 
     const fullPath = this.getPath(locale, path);
     if (!this.cache.hasOwnProperty(fullPath)) {
-      return this.indexed ? false : undefined;
+      return undefined;
     }
-    if (this.cache[fullPath] === false) {
+
+    if (this.cache[fullPath] === null) {
       return false;
     }
-    if (this.cache[fullPath].then) {
-      return undefined;
-    }
     return true;
   }
 
-  fetchFile(locale, path) {
+  async fetchFile(locale, path) {
     if (!this.locales.includes(locale)) {
-      return Promise.reject(`The source has no resources for locale "${locale}"`);
+      return null;
     }
 
     const fullPath = this.getPath(locale, path);
+    if (this.hasFile(locale, path) === undefined) {
+      let file = await L10nRegistry.load(fullPath);
 
-    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];
-      }
-    } else {
-      if (this.indexed) {
-        return Promise.reject(`The source has no resources for path "${fullPath}"`);
+      if (file === undefined) {
+        this.cache[fullPath] = null;
+      } else {
+        this.cache[fullPath] = file;
       }
     }
-    return this.cache[fullPath] = L10nRegistry.load(fullPath).then(
-      data => {
-        return this.cache[fullPath] = data;
-      },
-      err => {
-        this.cache[fullPath] = false;
-        return Promise.reject(err);
-      }
-    );
+    return this.cache[fullPath];
   }
 }
 
 /**
  * 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
-   *
-   * @returns {IndexedFileSource}
-   */
   constructor(name, locales, prePath, paths) {
     super(name, locales, prePath);
-    this.indexed = true;
-    for (const path of paths) {
-      this.cache[path] = true;
+    this.paths = paths;
+  }
+
+  hasFile(locale, path) {
+    if (!this.locales.includes(locale)) {
+      return false;
+    }
+    const fullPath = this.getPath(locale, path);
+    return this.paths.includes(fullPath);
+  }
+
+  async fetchFile(locale, path) {
+    if (!this.locales.includes(locale)) {
+      return null;
+    }
+
+    const fullPath = this.getPath(locale, path);
+    if (this.paths.includes(fullPath)) {
+      let file = await L10nRegistry.load(fullPath);
+
+      if (file === undefined) {
+        return null;
+      } else {
+        return file;
+      }
+    } else {
+      return null;
     }
   }
 }
 
 /**
- * The low level wrapper around Fetch API. It unifies the error scenarios to
- * always produce a promise rejection.
- *
  * We keep it as a method to make it easier to override for testing purposes.
- *
- * @param {string} url
- *
- * @returns {Promise<string>}
- */
+ **/
 L10nRegistry.load = function(url) {
-  return fetch(url).then(response => {
-    if (!response.ok) {
-      return Promise.reject(response.statusText);
-    }
-    return response.text()
-  });
+  return fetch(url).then(data => data.text()).catch(() => undefined);
 };
 
 this.L10nRegistry = L10nRegistry;
 this.FileSource = FileSource;
 this.IndexedFileSource = IndexedFileSource;
 
 this.EXPORTED_SYMBOLS = ['L10nRegistry', 'FileSource', 'IndexedFileSource'];
--- a/intl/l10n/test/test_l10nregistry.js
+++ b/intl/l10n/test/test_l10nregistry.js
@@ -1,23 +1,19 @@
 /* Any copyrighequal dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 const {
   L10nRegistry,
   FileSource,
   IndexedFileSource
 } = Components.utils.import("resource://gre/modules/L10nRegistry.jsm", {});
-Components.utils.import("resource://gre/modules/Timer.jsm");
 
 let fs;
 L10nRegistry.load = async function(url) {
-  if (!fs.hasOwnProperty(url)) {
-    return Promise.reject('Resource unavailable');
-  }
   return fs[url];
 }
 
 add_task(function test_methods_presence() {
   equal(typeof L10nRegistry.generateContexts, "function");
   equal(typeof L10nRegistry.getAvailableLocales, "function");
   equal(typeof L10nRegistry.registerSource, "function");
   equal(typeof L10nRegistry.updateSource, "function");
@@ -26,23 +22,24 @@ add_task(function test_methods_presence(
 /**
  * This test tests generation of a proper context for a single
  * source scenario
  */
 add_task(async function test_methods_calling() {
   fs = {
     '/localization/en-US/browser/menu.ftl': 'key = Value',
   };
+  const originalLoad = L10nRegistry.load;
 
   const source = new FileSource('test', ['en-US'], '/localization/{locale}');
   L10nRegistry.registerSource(source);
 
   const ctxs = L10nRegistry.generateContexts(['en-US'], ['/browser/menu.ftl']);
 
-  const ctx = (await ctxs.next()).value;
+  const ctx = await ctxs.next().value;
 
   equal(ctx.hasMessage('key'), true);
 
   // cleanup
   L10nRegistry.sources.clear();
   L10nRegistry.ctxCache.clear();
 });
 
@@ -62,27 +59,27 @@ add_task(async function test_has_one_sou
 
   equal(L10nRegistry.sources.size, 1);
   equal(L10nRegistry.sources.has('app'), true);
 
 
   // returns a single context
 
   let ctxs = L10nRegistry.generateContexts(['en-US'], ['test.ftl']);
-  let ctx0 = (await ctxs.next()).value;
+  let ctx0 = await ctxs.next().value;
   equal(ctx0.hasMessage('key'), true);
 
-  equal((await ctxs.next()).done, true);
+  equal(ctxs.next().done, true);
 
 
   // returns no contexts for missing locale
 
   ctxs = L10nRegistry.generateContexts(['pl'], ['test.ftl']);
 
-  equal((await ctxs.next()).done, true);
+  equal(ctxs.next().done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
   L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that public methods return expected values
@@ -105,41 +102,41 @@ add_task(async function test_has_two_sou
   equal(L10nRegistry.sources.size, 2);
   equal(L10nRegistry.sources.has('app'), true);
   equal(L10nRegistry.sources.has('platform'), true);
 
 
   // returns correct contexts for en-US
 
   let ctxs = L10nRegistry.generateContexts(['en-US'], ['test.ftl']);
-  let ctx0 = (await ctxs.next()).value;
+  let ctx0 = await ctxs.next().value;
 
   equal(ctx0.hasMessage('key'), true);
   let msg = ctx0.getMessage('key');
   equal(ctx0.format(msg), 'platform value');
 
-  equal((await ctxs.next()).done, true);
+  equal(ctxs.next().done, true);
 
 
   // returns correct contexts for [pl, en-US]
 
   ctxs = L10nRegistry.generateContexts(['pl', 'en-US'], ['test.ftl']);
-  ctx0 = (await ctxs.next()).value;
+  ctx0 = await ctxs.next().value;
   equal(ctx0.locales[0], 'pl');
   equal(ctx0.hasMessage('key'), true);
   let msg0 = ctx0.getMessage('key');
   equal(ctx0.format(msg0), 'app value');
 
-  let ctx1 = (await ctxs.next()).value;
+  let ctx1 = await ctxs.next().value;
   equal(ctx1.locales[0], 'en-US');
   equal(ctx1.hasMessage('key'), true);
   let msg1 = ctx1.getMessage('key');
   equal(ctx1.format(msg1), 'platform value');
 
-  equal((await ctxs.next()).done, true);
+  equal(ctxs.next().done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
   L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that behavior specific to the IndexedFileSource
@@ -186,29 +183,29 @@ add_task(async function test_override() 
     '/app/data/locales/pl/test.ftl': 'key = value',
     '/data/locales/pl/test.ftl': 'key = addon value'
   };
 
   equal(L10nRegistry.sources.size, 2);
   equal(L10nRegistry.sources.has('langpack-pl'), true);
 
   let ctxs = L10nRegistry.generateContexts(['pl'], ['test.ftl']);
-  let ctx0 = (await ctxs.next()).value;
+  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), 'addon value');
 
-  let ctx1 = (await ctxs.next()).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), 'value');
 
-  equal((await ctxs.next()).done, true);
+  equal(ctxs.next().done, true);
 
   // cleanup
   L10nRegistry.sources.clear();
   L10nRegistry.ctxCache.clear();
 });
 
 /**
  * This test verifies that new contexts are returned
@@ -219,32 +216,32 @@ add_task(async function test_updating() 
     '/data/locales/pl/test.ftl',
   ]);
   L10nRegistry.registerSource(oneSource);
   fs = {
     '/data/locales/pl/test.ftl': 'key = value'
   };
 
   let ctxs = L10nRegistry.generateContexts(['pl'], ['test.ftl']);
-  let ctx0 = (await ctxs.next()).value;
+  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), 'value');
 
 
   const newSource = new IndexedFileSource('langpack-pl', ['pl'], '/data/locales/{locale}/', [
     '/data/locales/pl/test.ftl'
   ]);
   fs['/data/locales/pl/test.ftl'] = 'key = new value';
   L10nRegistry.updateSource(newSource);
 
   equal(L10nRegistry.sources.size, 1);
   ctxs = L10nRegistry.generateContexts(['pl'], ['test.ftl']);
-  ctx0 = (await ctxs.next()).value;
+  ctx0 = await ctxs.next().value;
   msg0 = ctx0.getMessage('key');
   equal(ctx0.format(msg0), 'new value');
 
   // cleanup
   L10nRegistry.sources.clear();
   L10nRegistry.ctxCache.clear();
 });
 
@@ -265,151 +262,51 @@ add_task(async function test_removing() 
     '/app/data/locales/pl/test.ftl': 'key = value',
     '/data/locales/pl/test.ftl': 'key = addon value'
   };
 
   equal(L10nRegistry.sources.size, 2);
   equal(L10nRegistry.sources.has('langpack-pl'), true);
 
   let ctxs = L10nRegistry.generateContexts(['pl'], ['test.ftl']);
-  let ctx0 = (await ctxs.next()).value;
+  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), 'addon value');
 
-  let ctx1 = (await ctxs.next()).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), 'value');
 
-  equal((await ctxs.next()).done, true);
+  equal(ctxs.next().done, true);
 
   // Remove langpack
 
   L10nRegistry.removeSource('langpack-pl');
 
   equal(L10nRegistry.sources.size, 1);
   equal(L10nRegistry.sources.has('langpack-pl'), false);
 
   ctxs = L10nRegistry.generateContexts(['pl'], ['test.ftl']);
-  ctx0 = (await ctxs.next()).value;
+  ctx0 = await ctxs.next().value;
   equal(ctx0.locales[0], 'pl');
   equal(ctx0.hasMessage('key'), true);
   msg0 = ctx0.getMessage('key');
   equal(ctx0.format(msg0), 'value');
 
-  equal((await ctxs.next()).done, true);
+  equal(ctxs.next().done, true);
 
   // Remove app source
 
   L10nRegistry.removeSource('app');
 
   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}/');
-  L10nRegistry.registerSource(oneSource);
-  let twoSource = new FileSource('platform', ['en-US'], './platform/data/locales/{locale}/');
-  L10nRegistry.registerSource(twoSource);
-
-  fs = {
-    './app/data/locales/en-US/test.ftl': 'key = value en-US',
-    './platform/data/locales/en-US/test.ftl': 'key = value en-US',
-    './platform/data/locales/en-US/test2.ftl': 'key2 = value2 en-US'
-  };
-
-
-  // has two sources
-
-  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']);
-  let ctx0 = (await ctxs.next()).value;
-  let ctx1 = (await ctxs.next()).value;
-
-  equal((await ctxs.next()).done, true);
-
+  equal(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() {
-  /* eslint-disable mozilla/no-arbitrary-setTimeout */
-  let originalLoad = L10nRegistry.load;
-  let fetchIndex = new Map();
-
-  L10nRegistry.load = function(url) {
-    if (!fetchIndex.has(url)) {
-      fetchIndex.set(url, 0);
-    }
-    fetchIndex.set(url, fetchIndex.get(url) + 1);
-
-    if (url === '/en-US/slow-file.ftl') {
-      return new Promise((resolve, reject) => {
-        setTimeout(() => {
-          // Despite slow-file being the first on the list,
-          // by the time the it finishes loading, the other
-          // two files are already fetched.
-          equal(fetchIndex.get('/en-US/test.ftl'), 1);
-          equal(fetchIndex.get('/en-US/test2.ftl'), 1);
-
-          resolve('');
-        }, 10);
-      });
-    };
-    return Promise.resolve('');
-  }
-  let oneSource = new FileSource('app', ['en-US'], '/{locale}/');
-  L10nRegistry.registerSource(oneSource);
-
-  fs = {
-    '/en-US/test.ftl': 'key = value en-US',
-    '/en-US/test2.ftl': 'key2 = value2 en-US',
-    '/en-US/slow-file.ftl': 'key-slow = value slow en-US',
-  };
-
-  // returns a single context
-
-  let ctxs = L10nRegistry.generateContexts(['en-US'], ['slow-file.ftl', 'test.ftl', 'test2.ftl']);
-
-  equal(fetchIndex.size, 0);
-
-  let ctx0 = await ctxs.next();
-
-  equal(ctx0.done, false);
-
-  equal((await ctxs.next()).done, true);
-
-  // When requested again, the cache should make the load operation not
-  // increase the fetchedIndex count
-  let ctxs2= L10nRegistry.generateContexts(['en-US'], ['test.ftl', 'test2.ftl', 'slow-file.ftl']);
-
-  // cleanup
-  L10nRegistry.sources.clear();
-  L10nRegistry.ctxCache.clear();
-  L10nRegistry.load = originalLoad;
-});