Bug 1441135 - Stash all data needed for langpack startup in the startupCache. r?aswan draft
authorZibi Braniecki <zbraniecki@mozilla.com>
Tue, 27 Feb 2018 10:55:19 -0800
changeset 761494 d20cfeb0c9f0b6dc17920a890f4bc52112eb0bd4
parent 760935 ee326c976eebdca48128054022c443d3993e12b0
push id100960
push userbmo:gandalf@aviary.pl
push dateThu, 01 Mar 2018 03:42:17 +0000
reviewersaswan
bugs1441135
milestone60.0a1
Bug 1441135 - Stash all data needed for langpack startup in the startupCache. r?aswan MozReview-Commit-ID: 763VkrRGvCq
toolkit/components/extensions/Extension.jsm
toolkit/mozapps/extensions/test/xpcshell/test_webextension_langpack.js
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -626,33 +626,68 @@ class ExtensionData {
       }
 
       // Normalize all patterns to contain a single leading /
       if (manifest.web_accessible_resources) {
         webAccessibleResources.push(...manifest.web_accessible_resources
           .map(path => path.replace(/^\/*/, "/")));
       }
     } else if (this.type == "langpack") {
-      // Compute the chrome resources to be registered for this langpack
-      // and stash them in startupData
+      // Langpack startup is performance critical, so we want to compute as much
+      // as possible here to make startup not trigger async DB reads.
+      // We'll store the four items below in the startupData.
+
+      // 1. Compute the chrome resources to be registered for this langpack.
       const platform = AppConstants.platform;
       const chromeEntries = [];
       for (const [language, entry] of Object.entries(manifest.languages)) {
         for (const [alias, path] of Object.entries(entry.chrome_resources || {})) {
           if (typeof path === "string") {
             chromeEntries.push(["locale", alias, language, path]);
           } else if (platform in path) {
             // If the path is not a string, it's an object with path per
             // platform where the keys are taken from AppConstants.platform
             chromeEntries.push(["locale", alias, language, path[platform]]);
           }
         }
       }
 
-      this.startupData = {chromeEntries};
+
+      // 2. Compute langpack ID.
+      const productCodeName = AppConstants.MOZ_BUILD_APP.replace("/", "-");
+
+      // The result path looks like this:
+      //   Firefox - `langpack-pl-browser`
+      //   Fennec - `langpack-pl-mobile-android`
+      const langpackId =
+        `langpack-${manifest.langpack_id}-${productCodeName}`;
+
+
+      // 3. Compute L10nRegistry sources for this langpack.
+      const l10nRegistrySources = {};
+
+      // Check if there's a root directory `/localization` in the langpack.
+      // If there is one, add it with the name `toolkit` as a FileSource.
+      const entries = await this.readDirectory("localization");
+      if (entries.length > 0) {
+        l10nRegistrySources.toolkit = "";
+      }
+
+      // Add any additional sources listed in the manifest
+      if (manifest.sources) {
+        for (const [sourceName, {base_path}] of Object.entries(manifest.sources)) {
+          l10nRegistrySources[sourceName] = base_path;
+        }
+      }
+
+      // 4. Save the list of languages handled by this langpack.
+      const languages = Object.keys(manifest.languages);
+
+
+      this.startupData = {chromeEntries, langpackId, l10nRegistrySources, languages};
     }
 
     if (schemaPromises.size) {
       let schemas = new Map();
       for (let [url, promise] of schemaPromises) {
         schemas.set(url, await promise);
       }
       result.schemaURLs = schemas;
@@ -1768,84 +1803,50 @@ class Langpack extends ExtensionData {
   readLocaleFile(locale) {
     return StartupCache.locales.get([this.id, this.version, locale],
                                     () => super.readLocaleFile(locale))
       .then(result => {
         this.localeData.messages.set(locale, result);
       });
   }
 
-  async _parseManifest() {
-    let data = await super.parseManifest();
-
-    const productCodeName = AppConstants.MOZ_BUILD_APP.replace("/", "-");
-
-    // The result path looks like this:
-    //   Firefox - `langpack-pl-browser`
-    //   Fennec - `langpack-pl-mobile-android`
-    data.langpackId =
-      `langpack-${data.manifest.langpack_id}-${productCodeName}`;
-
-    const l10nRegistrySources = {};
-
-    // Check if there's a root directory `/localization` in the langpack.
-    // If there is one, add it with the name `toolkit` as a FileSource.
-    const entries = await this.readDirectory("localization");
-    if (entries.length > 0) {
-      l10nRegistrySources.toolkit = "";
-    }
-
-    // Add any additional sources listed in the manifest
-    if (data.manifest.sources) {
-      for (const [sourceName, {base_path}] of Object.entries(data.manifest.sources)) {
-        l10nRegistrySources[sourceName] = base_path;
-      }
-    }
-
-    data.l10nRegistrySources = l10nRegistrySources;
-
-    return data;
-  }
-
   parseManifest() {
     return StartupCache.manifests.get(this.manifestCacheKey,
-                                      () => this._parseManifest());
+                                      () => super.parseManifest());
   }
 
   async startup(reason) {
     this.chromeRegistryHandle = null;
     if (this.startupData.chromeEntries.length > 0) {
       const manifestURI = Services.io.newURI("manifest.json", null, this.rootURI);
       this.chromeRegistryHandle =
         aomStartup.registerChrome(manifestURI, this.startupData.chromeEntries);
     }
 
-    const data = await this.parseManifest();
-    this.langpackId = data.langpackId;
-    this.l10nRegistrySources = data.l10nRegistrySources;
+    const langpackId = this.startupData.langpackId;
+    const l10nRegistrySources = this.startupData.l10nRegistrySources;
+
+    resourceProtocol.setSubstitution(langpackId, this.rootURI);
 
-    const languages = Object.keys(data.manifest.languages);
-    resourceProtocol.setSubstitution(this.langpackId, this.rootURI);
-
-    for (const [sourceName, basePath] of Object.entries(this.l10nRegistrySources)) {
+    for (const [sourceName, basePath] of Object.entries(l10nRegistrySources)) {
       L10nRegistry.registerSource(new FileSource(
-        `${sourceName}-${this.langpackId}`,
-        languages,
-        `resource://${this.langpackId}/${basePath}localization/{locale}/`
+        `${sourceName}-${langpackId}`,
+        this.startupData.languages,
+        `resource://${langpackId}/${basePath}localization/{locale}/`
       ));
     }
 
     Services.obs.notifyObservers({wrappedJSObject: {langpack: this}},
                                  "webextension-langpack-startup");
   }
 
   async shutdown(reason) {
-    for (const sourceName of Object.keys(this.l10nRegistrySources)) {
-      L10nRegistry.removeSource(`${sourceName}-${this.langpackId}`);
+    for (const sourceName of Object.keys(this.startupData.l10nRegistrySources)) {
+      L10nRegistry.removeSource(`${sourceName}-${this.startupData.langpackId}`);
     }
     if (this.chromeRegistryHandle) {
       this.chromeRegistryHandle.destruct();
       this.chromeRegistryHandle = null;
     }
 
-    resourceProtocol.setSubstitution(this.langpackId, null);
+    resourceProtocol.setSubstitution(this.startupData.langpackId, null);
   }
 }
--- a/toolkit/mozapps/extensions/test/xpcshell/test_webextension_langpack.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_webextension_langpack.js
@@ -30,33 +30,37 @@ function promiseLangpackStartup() {
  * the language pack registers and unregisters correct
  * languages at various stages.
  */
 add_task(async function() {
   // Make sure that `und` locale is not installed.
   equal(L10nRegistry.getAvailableLocales().includes("und"), false);
   equal(Services.locale.getAvailableLocales().includes("und"), false);
 
-  let [{addon}, ] = await Promise.all([
+  let [, {addon}] = await Promise.all([
+    promiseLangpackStartup(),
     promiseInstallFile(do_get_addon("langpack_1"), true),
-    promiseLangpackStartup()
   ]);
 
   // Now make sure that `und` locale is available.
   equal(L10nRegistry.getAvailableLocales().includes("und"), true);
   equal(Services.locale.getAvailableLocales().includes("und"), true);
 
   addon.userDisabled = true;
 
   // It is not available after the langpack has been disabled.
   equal(L10nRegistry.getAvailableLocales().includes("und"), false);
   equal(Services.locale.getAvailableLocales().includes("und"), false);
 
-  addon.userDisabled = false;
-  await promiseLangpackStartup();
+  // This quirky code here allows us to handle a scenario where enabling the
+  // addon is synchronous or asynchronous.
+  await Promise.all([
+    promiseLangpackStartup(),
+    (() => { addon.userDisabled = false; })()
+  ]);
 
   // After re-enabling it, the `und` locale is available again.
   equal(L10nRegistry.getAvailableLocales().includes("und"), true);
   equal(Services.locale.getAvailableLocales().includes("und"), true);
 
   addon.uninstall();
 
   // After the langpack has been uninstalled, no more `und` in locales.
@@ -64,19 +68,19 @@ add_task(async function() {
   equal(Services.locale.getAvailableLocales().includes("und"), false);
 });
 
 /**
  * This test verifies that registries are able to load and return
  * correct strings available in the language pack.
  */
 add_task(async function() {
-  let [{addon}, ] = await Promise.all([
+  let [, {addon}] = await Promise.all([
+    promiseLangpackStartup(),
     promiseInstallFile(do_get_addon("langpack_1"), true),
-    promiseLangpackStartup()
   ]);
 
   {
     // Toolkit string
     let ctxs = L10nRegistry.generateContexts(["und"], ["toolkit_test.ftl"]);
     let ctx0 = (await ctxs.next()).value;
     equal(ctx0.hasMessage("message-id1"), true);
   }
@@ -105,19 +109,19 @@ add_task(async function() {
   addon.uninstall();
 });
 
 /**
  * This test verifies that language pack will get disabled after app
  * gets upgraded.
  */
 add_task(async function() {
-  let [{addon}, ] = await Promise.all([
+  let [, {addon}] = await Promise.all([
+    promiseLangpackStartup(),
     promiseInstallFile(do_get_addon("langpack_1"), true),
-    promiseLangpackStartup()
   ]);
   Assert.ok(addon.isActive);
 
   await promiseShutdownManager();
 
   gAppInfo.version = "59";
   gAppInfo.platformVersion = "59";