Bug 1869678 - Use more idiomatic code patterns for better type infere…

…nce r=robwu

These fall under 5 main categories:

 1) Declare and/or initialize all class fiels in the constructor.
    (general good practise)

 2) Use real getters and redefineGetter instead of defineLazyGetter.
    (also keeps related code closer together)

 3) When subclassing, don't override class fields with getters (or vice versa).
    microsoft/TypeScript#33509

 4) Declare and assign object literals at the same time, not separatelly.
    (don't use `let foo;` at the top of the file, use `var foo = {`)

 5) Don't re-use local variables unnecesarily with different types.
    (general good practise, local variables are "free")

Differential Revision: https://phabricator.services.mozilla.com/D196386

toolkit/components/extensions/.eslintrc.js

@@ -56,8 +56,22 @@ module.exports = {
       },
     ],
 
-    // No using variables before defined
-    "no-use-before-define": "error",
+    // No using things before they're defined.
+    "no-use-before-define": [
+      "error",
+      {
+        allowNamedExports: true,
+        classes: true,
+        // The next two being false allows idiomatic patterns which are more
+        // type-inference friendly.  Functions are hoisted, so this is safe.
+        functions: false,
+        // This flag is only meaningful for `var` declarations.
+        // When false, it still disallows use-before-define in the same scope.
+        // Since we only allow `var` at the global scope, this is no worse than
+        // how we currently declare an uninitialized `let` at the top of file.
+        variables: false,
+      },
+    ],
 
     // Disallow using variables outside the blocks they are defined (especially
     // since only let and const are used, see "no-var").

toolkit/components/extensions/ConduitsParent.sys.mjs

@@ -452,21 +452,21 @@ export class ConduitsParent extends JSWindowActorParent {
       return Hub.recvConduitOpened(arg, this);
     }
 
-    sender = Hub.remotes.get(sender);
-    if (!sender || sender.actor !== this) {
+    let remote = Hub.remotes.get(sender);
+    if (!remote || remote.actor !== this) {
       throw new Error(`Unknown sender or wrong actor for recv${name}`);
     }
 
     if (name === "ConduitClosed") {
-      return Hub.recvConduitClosed(sender);
+      return Hub.recvConduitClosed(remote);
     }
 
     let conduit = Hub.byMethod.get(name);
     if (!conduit) {
       throw new Error(`Parent conduit for recv${name} not found`);
     }
 
-    return conduit._recv(name, arg, { actor: this, query, sender });
+    return conduit._recv(name, arg, { actor: this, query, sender: remote });
   }
 
   /**

toolkit/components/extensions/Extension.sys.mjs

@@ -172,7 +172,7 @@ export { Management };
 
 const { getUniqueId, promiseTimeout } = ExtensionUtils;
 
-const { EventEmitter, updateAllowedOrigins } = ExtensionCommon;
+const { EventEmitter, redefineGetter, updateAllowedOrigins } = ExtensionCommon;
 
 ChromeUtils.defineLazyGetter(
   lazy,
@@ -869,6 +869,19 @@ const manifestTypes = new Map([
  * `loadManifest` has been called, and completed.
  */
 export class ExtensionData {
+  /**
+   * Note: These fields are only available and meant to be used on Extension
+   * instances, declared here because methods from this class reference them.
+   */
+  /** @type {object} TODO: move to the Extension class, bug 1871094. */
+  addonData;
+  /** @type {nsIURI} */
+  baseURI;
+  /** @type {nsIPrincipal} */
+  principal;
+  /** @type {boolean} */
+  temporarilyInstalled;
+
   constructor(rootURI, isPrivileged = false) {
     this.rootURI = rootURI;
     this.resourceURL = rootURI.spec;
@@ -2676,7 +2689,7 @@ class BootstrapScope {
     // APP_STARTED.  In some situations, such as background and
     // persisted listeners, we also need to know that the addon
     // was updated.
-    this.updateReason = this.BOOTSTRAP_REASON_TO_STRING_MAP[reason];
+    this.updateReason = BootstrapScope.BOOTSTRAP_REASON_MAP[reason];
     // Retain any previously granted permissions that may have migrated
     // into the optional list.
     if (data.oldPermissions) {
@@ -2703,39 +2716,35 @@ class BootstrapScope {
     // eslint-disable-next-line no-use-before-define
     this.extension = new Extension(
       data,
-      this.BOOTSTRAP_REASON_TO_STRING_MAP[reason],
+      BootstrapScope.BOOTSTRAP_REASON_MAP[reason],
       this.updateReason
     );
     return this.extension.startup();
   }
 
   async shutdown(data, reason) {
     let result = await this.extension.shutdown(
-      this.BOOTSTRAP_REASON_TO_STRING_MAP[reason]
+      BootstrapScope.BOOTSTRAP_REASON_MAP[reason]
     );
     this.extension = null;
     return result;
   }
-}
 
-ChromeUtils.defineLazyGetter(
-  BootstrapScope.prototype,
-  "BOOTSTRAP_REASON_TO_STRING_MAP",
-  () => {
-    const { BOOTSTRAP_REASONS } = lazy.AddonManagerPrivate;
-
-    return Object.freeze({
-      [BOOTSTRAP_REASONS.APP_STARTUP]: "APP_STARTUP",
-      [BOOTSTRAP_REASONS.APP_SHUTDOWN]: "APP_SHUTDOWN",
-      [BOOTSTRAP_REASONS.ADDON_ENABLE]: "ADDON_ENABLE",
-      [BOOTSTRAP_REASONS.ADDON_DISABLE]: "ADDON_DISABLE",
-      [BOOTSTRAP_REASONS.ADDON_INSTALL]: "ADDON_INSTALL",
-      [BOOTSTRAP_REASONS.ADDON_UNINSTALL]: "ADDON_UNINSTALL",
-      [BOOTSTRAP_REASONS.ADDON_UPGRADE]: "ADDON_UPGRADE",
-      [BOOTSTRAP_REASONS.ADDON_DOWNGRADE]: "ADDON_DOWNGRADE",
+  static get BOOTSTRAP_REASON_MAP() {
+    const BR = lazy.AddonManagerPrivate.BOOTSTRAP_REASONS;
+    const value = Object.freeze({
+      [BR.APP_STARTUP]: "APP_STARTUP",
+      [BR.APP_SHUTDOWN]: "APP_SHUTDOWN",
+      [BR.ADDON_ENABLE]: "ADDON_ENABLE",
+      [BR.ADDON_DISABLE]: "ADDON_DISABLE",
+      [BR.ADDON_INSTALL]: "ADDON_INSTALL",
+      [BR.ADDON_UNINSTALL]: "ADDON_UNINSTALL",
+      [BR.ADDON_UPGRADE]: "ADDON_UPGRADE",
+      [BR.ADDON_DOWNGRADE]: "ADDON_DOWNGRADE",
     });
+    return redefineGetter(this, "BOOTSTRAP_REASON_TO_STRING_MAP", value);
   }
-);
+}
 
 class DictionaryBootstrapScope extends BootstrapScope {
   install(data, reason) {}
@@ -2744,28 +2753,28 @@ class DictionaryBootstrapScope extends BootstrapScope {
   startup(data, reason) {
     // eslint-disable-next-line no-use-before-define
     this.dictionary = new Dictionary(data);
-    return this.dictionary.startup(this.BOOTSTRAP_REASON_TO_STRING_MAP[reason]);
+    return this.dictionary.startup(BootstrapScope.BOOTSTRAP_REASON_MAP[reason]);
   }
 
-  shutdown(data, reason) {
-    this.dictionary.shutdown(this.BOOTSTRAP_REASON_TO_STRING_MAP[reason]);
+  async shutdown(data, reason) {
+    this.dictionary.shutdown(BootstrapScope.BOOTSTRAP_REASON_MAP[reason]);
     this.dictionary = null;
   }
 }
 
 class LangpackBootstrapScope extends BootstrapScope {
   install(data, reason) {}
   uninstall(data, reason) {}
-  update(data, reason) {}
+  async update(data, reason) {}
 
   startup(data, reason) {
     // eslint-disable-next-line no-use-before-define
     this.langpack = new Langpack(data);
-    return this.langpack.startup(this.BOOTSTRAP_REASON_TO_STRING_MAP[reason]);
+    return this.langpack.startup(BootstrapScope.BOOTSTRAP_REASON_MAP[reason]);
   }
 
-  shutdown(data, reason) {
-    this.langpack.shutdown(this.BOOTSTRAP_REASON_TO_STRING_MAP[reason]);
+  async shutdown(data, reason) {
+    this.langpack.shutdown(BootstrapScope.BOOTSTRAP_REASON_MAP[reason]);
     this.langpack = null;
   }
 }
@@ -2779,12 +2788,12 @@ class SitePermissionBootstrapScope extends BootstrapScope {
     // eslint-disable-next-line no-use-before-define
     this.sitepermission = new SitePermission(data);
     return this.sitepermission.startup(
-      this.BOOTSTRAP_REASON_TO_STRING_MAP[reason]
+      BootstrapScope.BOOTSTRAP_REASON_MAP[reason]
     );
   }
 
-  shutdown(data, reason) {
-    this.sitepermission.shutdown(this.BOOTSTRAP_REASON_TO_STRING_MAP[reason]);
+  async shutdown(data, reason) {
+    this.sitepermission.shutdown(BootstrapScope.BOOTSTRAP_REASON_MAP[reason]);
     this.sitepermission = null;
   }
 }
@@ -2800,6 +2809,9 @@ let pendingExtensions = new Map();
  * @augments ExtensionData
  */
 export class Extension extends ExtensionData {
+  /** @type {Map<string, Map<string, any>>} */
+  persistentListeners;
+
   constructor(addonData, startupReason, updateReason) {
     super(addonData.resourceURI, addonData.isPrivileged);
 
@@ -2832,6 +2844,7 @@ export class Extension extends ExtensionData {
     this.startupData = addonData.startupData || {};
     this.startupReason = startupReason;
     this.updateReason = updateReason;
+    this.temporarilyInstalled = !!addonData.temporarilyInstalled;
 
     if (
       updateReason ||
@@ -3077,10 +3090,6 @@ export class Extension extends ExtensionData {
     return [this.id, this.version, Services.locale.appLocaleAsBCP47];
   }
 
-  get temporarilyInstalled() {
-    return !!this.addonData.temporarilyInstalled;
-  }
-
   saveStartupData() {
     if (this.dontSaveStartupData) {
       return;

toolkit/components/extensions/ExtensionActions.sys.mjs

@@ -43,6 +43,7 @@ class PanelActionBase {
       enabled: true,
       title: options.default_title || extension.name,
       popup: options.default_popup || "",
+      icon: null,
     };
     this.globals = Object.create(this.defaults);
 

toolkit/components/extensions/ExtensionChild.sys.mjs

@@ -39,7 +39,7 @@ import { ExtensionUtils } from "resource://gre/modules/ExtensionUtils.sys.mjs";
 const { DefaultMap, ExtensionError, LimitedSet, getUniqueId } = ExtensionUtils;
 
 const {
-  defineLazyGetter,
+  redefineGetter,
   EventEmitter,
   EventManager,
   LocalAPIImplementation,
@@ -299,12 +299,13 @@ class Port {
     }
     throw new this.context.Error("Attempt to postMessage on disconnected port");
   }
-}
 
-defineLazyGetter(Port.prototype, "api", function () {
-  let api = this.getAPI();
-  return Cu.cloneInto(api, this.context.cloneScope, { cloneFunctions: true });
-});
+  get api() {
+    const scope = this.context.cloneScope;
+    const value = Cu.cloneInto(this.getAPI(), scope, { cloneFunctions: true });
+    return redefineGetter(this, "api", value);
+  }
+}
 
 /**
  * Each extension context gets its own Messenger object. It handles the
@@ -522,7 +523,7 @@ class BrowserExtensionContent extends EventEmitter {
 
   emit(event, ...args) {
     Services.cpmm.sendAsyncMessage(this.MESSAGE_EMIT_EVENT, { event, args });
-    super.emit(event, ...args);
+    return super.emit(event, ...args);
   }
 
   // TODO(Bug 1768471): consider folding this back into emit if we will change it to
@@ -914,10 +915,10 @@ class ChildAPIManager {
    *   hasListener methods. See SchemaAPIInterface for documentation.
    */
   getParentEvent(path) {
-    path = path.split(".");
+    let parts = path.split(".");
 
-    let name = path.pop();
-    let namespace = path.join(".");
+    let name = parts.pop();
+    let namespace = parts.join(".");
 
     let impl = new ProxyAPIImplementation(namespace, name, this, true);
     return {