Try to make the Preferences/AppOptions initialization slightly mo…

…re efficient *Please note:* This patch contains a couple of micro-optimizations, hence I understand if it's deemed unnecessary. Move the `AppOptions` initialization into the `Preferences` constructor, since that allows us to remove a couple of function calls, a bit of asynchronicity and one a loop that's currently happening in the early stages of the default viewer initialization. Finally, move the `Preferences` initialization to occurs a *tiny* bit earlier since that cannot hurt given that the entire viewer initialization depends on it being available.
mozilla · Oct 16, 2023 · b15af61 · b15af61
1 parent a9a1195
commit b15af61
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 57 deletions.
diff --git a/web/app.js b/web/app.js
@@ -234,7 +234,6 @@ const PDFViewerApplication = {
 
   // Called once when the document is loaded.
   async initialize(appConfig) {
-    this.preferences = this.externalServices.createPreferences();
     this.appConfig = appConfig;
 
     if (
@@ -245,7 +244,16 @@ const PDFViewerApplication = {
       this._nimbusDataPromise = this.externalServices.getNimbusExperimentData();
     }
 
-    await this._initializeOptions();
+    // Ensure that `Preferences`, and indirectly `AppOptions`, have initialized
+    // before creating e.g. the various viewer components.
+    try {
+      await this.preferences.initializedPromise;
+    } catch (ex) {
+      console.error(`initialize: "${ex.message}".`);
+    }
+    if (AppOptions.get("pdfBugEnabled")) {
+      await this._parseHashParams();
+    }
     this._forceCssTheme();
     await this._initializeL10n();
 
@@ -275,37 +283,6 @@ const PDFViewerApplication = {
     this._initializedCapability.resolve();
   },
 
-  /**
-   * @private
-   */
-  async _initializeOptions() {
-    if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
-      if (AppOptions.get("disablePreferences")) {
-        if (AppOptions.get("pdfBugEnabled")) {
-          await this._parseHashParams();
-        }
-        // Give custom implementations of the default viewer a simpler way to
-        // opt-out of having the `Preferences` override existing `AppOptions`.
-        return;
-      }
-      if (AppOptions._hasUserOptions()) {
-        console.warn(
-          "_initializeOptions: The Preferences may override manually set AppOptions; " +
-            'please use the "disablePreferences"-option in order to prevent that.'
-        );
-      }
-    }
-    try {
-      AppOptions.setAll(await this.preferences.getAll());
-    } catch (reason) {
-      console.error(`_initializeOptions: "${reason.message}".`);
-    }
-
-    if (AppOptions.get("pdfBugEnabled")) {
-      await this._parseHashParams();
-    }
-  },
-
   /**
    * Potentially parse special debugging flags in the hash section of the URL.
    * @private
@@ -698,6 +675,7 @@ const PDFViewerApplication = {
   },
 
   async run(config) {
+    this.preferences = this.externalServices.createPreferences();
     await this.initialize(config);
 
     const { appConfig, eventBus } = this;

diff --git a/web/app_options.js b/web/app_options.js
@@ -402,7 +402,21 @@ class AppOptions {
     userOptions[name] = value;
   }
 
-  static setAll(options) {
+  static setAll(options, init = false) {
+    if ((typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) && init) {
+      if (this.get("disablePreferences")) {
+        // Give custom implementations of the default viewer a simpler way to
+        // opt-out of having the `Preferences` override existing `AppOptions`.
+        return;
+      }
+      if (Object.keys(userOptions).length) {
+        console.warn(
+          "setAll: The Preferences may override manually set AppOptions; " +
+            'please use the "disablePreferences"-option in order to prevent that.'
+        );
+      }
+    }
+
     for (const name in options) {
       userOptions[name] = options[name];
     }
@@ -413,10 +427,4 @@ class AppOptions {
   }
 }
 
-if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) {
-  AppOptions._hasUserOptions = function () {
-    return Object.keys(userOptions).length > 0;
-  };
-}
-
 export { AppOptions, compatibilityParams, OptionKind };
diff --git a/web/preferences.js b/web/preferences.js
@@ -47,12 +47,13 @@ class BasePreferences {
     this.#initializedPromise = this._readFromStorage(this.#defaults).then(
       prefs => {
         for (const name in this.#defaults) {
-          const prefValue = prefs?.[name];
+          const defaultValue = this.#defaults[name],
+            prefValue = prefs?.[name];
           // Ignore preferences whose types don't match the default values.
-          if (typeof prefValue === typeof this.#defaults[name]) {
-            this.#prefs[name] = prefValue;
-          }
+          this.#prefs[name] =
+            typeof prefValue === typeof defaultValue ? prefValue : defaultValue;
         }
+        AppOptions.setAll(this.#prefs, /* init = */ true);
       }
     );
   }
@@ -156,19 +157,8 @@ class BasePreferences {
     return this.#prefs[name] ?? defaultValue;
   }
 
-  /**
-   * Get the values of all preferences.
-   * @returns {Promise} A promise that is resolved with an {Object} containing
-   *                    the values of all preferences.
-   */
-  async getAll() {
-    await this.#initializedPromise;
-    const obj = Object.create(null);
-
-    for (const name in this.#defaults) {
-      obj[name] = this.#prefs[name] ?? this.#defaults[name];
-    }
-    return obj;
+  get initializedPromise() {
+    return this.#initializedPromise;
   }
 }