Bug 1203330 Part 3 Remove EventManager draft
authorAndrew Swan <aswan@mozilla.com>
Thu, 26 Jan 2017 11:27:31 -0800
changeset 467099 f49fd5060cbcb1a25035d6d20008f0207204aa53
parent 467098 aa0b2a64e4f4d3976b216721e21ae9ed95dde3f6
child 467492 762225b92843eeeb1180af0a48c16a426cc8cb8f
push id43100
push useraswan@mozilla.com
push dateFri, 27 Jan 2017 04:03:36 +0000
bugs1203330
milestone54.0a1
Bug 1203330 Part 3 Remove EventManager Also clean up some test warnings related to event listener IPC along the way. MozReview-Commit-ID: KOBQX3TynV
toolkit/components/extensions/ExtensionUtils.jsm
toolkit/components/extensions/test/xpcshell/test_ext_alarms.js
toolkit/components/extensions/test/xpcshell/test_ext_contexts.js
toolkit/components/extensions/test/xpcshell/test_ext_idle.js
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -575,127 +575,35 @@ LocaleData.prototype = {
     // Return the browser locale, but convert it to a Chrome-style
     // locale code.
     return Locale.getLocale().replace(/-/g, "_");
   },
 };
 
 // This is a generic class for managing event listeners. Example usage:
 //
-// new EventManager(context, "api.subAPI", fire => {
+// new SingletonEventManager(context, "api.subAPI", fire => {
 //   let listener = (...) => {
 //     // Fire any listeners registered with addListener.
-//     fire(arg1, arg2);
+//     fire.async(arg1, arg2);
 //   };
 //   // Register the listener.
 //   SomehowRegisterListener(listener);
 //   return () => {
 //     // Return a way to unregister the listener.
 //     SomehowUnregisterListener(listener);
 //   };
 // }).api()
 //
 // The result is an object with addListener, removeListener, and
 // hasListener methods. |context| is an add-on scope (either an
 // ExtensionContext in the chrome process or ExtensionContext in a
 // content process). |name| is for debugging. |register| is a function
-// to register the listener. |register| is only called once, even if
-// multiple listeners are registered. |register| should return an
+// to register the listener. |register| should return an
 // unregister function that will unregister the listener.
-function EventManager(context, name, register) {
-  this.context = context;
-  this.name = name;
-  this.register = register;
-  this.unregister = null;
-  this.callbacks = new Set();
-}
-
-EventManager.prototype = {
-  addListener(callback) {
-    if (typeof(callback) != "function") {
-      dump(`Expected function\n${Error().stack}`);
-      return;
-    }
-    if (this.context.unloaded) {
-      dump(`Cannot add listener to ${this.name} after context unloaded`);
-      return;
-    }
-
-    if (!this.callbacks.size) {
-      this.context.callOnClose(this);
-
-      let fireFunc = this.fire.bind(this);
-      let fireWithoutClone = this.fireWithoutClone.bind(this);
-      fireFunc.withoutClone = fireWithoutClone;
-      this.unregister = this.register(fireFunc);
-    }
-    this.callbacks.add(callback);
-  },
-
-  removeListener(callback) {
-    if (!this.callbacks.size) {
-      return;
-    }
-
-    this.callbacks.delete(callback);
-    if (this.callbacks.size == 0) {
-      this.unregister();
-      this.unregister = null;
-
-      this.context.forgetOnClose(this);
-    }
-  },
-
-  hasListener(callback) {
-    return this.callbacks.has(callback);
-  },
-
-  fire(...args) {
-    this._fireCommon("runSafe", args);
-  },
-
-  fireWithoutClone(...args) {
-    this._fireCommon("runSafeWithoutClone", args);
-  },
-
-  _fireCommon(runSafeMethod, args) {
-    for (let callback of this.callbacks) {
-      Promise.resolve(callback).then(callback => {
-        if (this.context.unloaded) {
-          dump(`${this.name} event fired after context unloaded.\n`);
-        } else if (!this.context.active) {
-          dump(`${this.name} event fired while context is inactive.\n`);
-        } else if (this.callbacks.has(callback)) {
-          this.context[runSafeMethod](callback, ...args);
-        }
-      });
-    }
-  },
-
-  close() {
-    if (this.callbacks.size) {
-      this.unregister();
-    }
-    this.callbacks.clear();
-    this.register = null;
-    this.unregister = null;
-  },
-
-  api() {
-    return {
-      addListener: callback => this.addListener(callback),
-      removeListener: callback => this.removeListener(callback),
-      hasListener: callback => this.hasListener(callback),
-    };
-  },
-};
-
-// Similar to EventManager, but it doesn't try to consolidate event
-// notifications. Each addListener call causes us to register once. It
-// allows extra arguments to be passed to addListener.
 function SingletonEventManager(context, name, register) {
   this.context = context;
   this.name = name;
   this.register = register;
   this.unregister = new Map();
 }
 
 SingletonEventManager.prototype = {
@@ -1237,17 +1145,16 @@ this.ExtensionUtils = {
   runSafe,
   runSafeSync,
   runSafeSyncWithoutClone,
   runSafeWithoutClone,
   stylesheetMap,
   DefaultMap,
   DefaultWeakMap,
   EventEmitter,
-  EventManager,
   ExtensionError,
   IconDetails,
   LocaleData,
   MessageManagerProxy,
   PlatformInfo,
   SingletonEventManager,
   SpreadArgs,
 };
--- a/toolkit/components/extensions/test/xpcshell/test_ext_alarms.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_alarms.js
@@ -47,20 +47,23 @@ add_task(function* test_alarm_fires() {
     background: `(${backgroundScript})()`,
     manifest: {
       permissions: ["alarms"],
     },
   });
 
   yield extension.startup();
   yield extension.awaitFinish("alarm-fires");
+
+  // Defer unloading the extension so the asynchronous event listener
+  // reply finishes.
+  yield new Promise(resolve => setTimeout(resolve, 0));
   yield extension.unload();
 });
 
-
 add_task(function* test_alarm_fires_with_when() {
   function backgroundScript() {
     let ALARM_NAME = "test_ext_alarms";
     let timer;
 
     browser.alarms.onAlarm.addListener(alarm => {
       browser.test.assertEq(ALARM_NAME, alarm.name, "alarm has the expected name");
       clearTimeout(timer);
@@ -81,20 +84,23 @@ add_task(function* test_alarm_fires_with
     background: `(${backgroundScript})()`,
     manifest: {
       permissions: ["alarms"],
     },
   });
 
   yield extension.startup();
   yield extension.awaitFinish("alarm-when");
+
+  // Defer unloading the extension so the asynchronous event listener
+  // reply finishes.
+  yield new Promise(resolve => setTimeout(resolve, 0));
   yield extension.unload();
 });
 
-
 add_task(function* test_alarm_clear_non_matching_name() {
   async function backgroundScript() {
     let ALARM_NAME = "test_ext_alarms";
 
     browser.alarms.create(ALARM_NAME, {when: Date.now() + 2000});
 
     let wasCleared = await browser.alarms.clear(ALARM_NAME + "1");
     browser.test.assertFalse(wasCleared, "alarm was not cleared");
@@ -111,17 +117,16 @@ add_task(function* test_alarm_clear_non_
     },
   });
 
   yield extension.startup();
   yield extension.awaitFinish("alarm-clear");
   yield extension.unload();
 });
 
-
 add_task(function* test_alarm_get_and_clear_single_argument() {
   async function backgroundScript() {
     browser.alarms.create({when: Date.now() + 2000});
 
     let alarm = await browser.alarms.get();
     browser.test.assertEq("", alarm.name, "expected alarm returned");
 
     let wasCleared = await browser.alarms.clear();
--- a/toolkit/components/extensions/test/xpcshell/test_ext_contexts.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_contexts.js
@@ -7,17 +7,16 @@ Cu.import("resource://gre/modules/Timer.
 Cu.import("resource://gre/modules/ExtensionCommon.jsm");
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 
 var {
   BaseContext,
 } = ExtensionCommon;
 
 var {
-  EventManager,
   SingletonEventManager,
 } = ExtensionUtils;
 
 class StubContext extends BaseContext {
   constructor() {
     let fakeExtension = {id: "test@web.extension"};
     super("testEnv", fakeExtension);
     this.sandbox = Cu.Sandbox(global);
@@ -60,70 +59,54 @@ add_task(function* test_post_unload_prom
   // any micro-tasks that get enqueued by the resolution handlers above.
   yield new Promise(resolve => setTimeout(resolve, 0));
 });
 
 
 add_task(function* test_post_unload_listeners() {
   let context = new StubContext();
 
-  let fireEvent;
-  let onEvent = new EventManager(context, "onEvent", fire => {
-    fireEvent = fire;
-    return () => {};
-  });
-
   let fireSingleton;
   let onSingleton = new SingletonEventManager(context, "onSingleton", fire => {
     fireSingleton = () => {
       fire.async();
     };
     return () => {};
   });
 
   let fail = event => {
     ok(false, `Unexpected event: ${event}`);
   };
 
-  // Check that event listeners aren't called after they've been removed.
-  onEvent.addListener(fail);
+  // Check that event listeners isn't called after it has been removed.
   onSingleton.addListener(fail);
 
-  let promises = [
-    new Promise(resolve => onEvent.addListener(resolve)),
-    new Promise(resolve => onSingleton.addListener(resolve)),
-  ];
+  let promise = new Promise(resolve => onSingleton.addListener(resolve));
 
-  fireEvent("onEvent");
   fireSingleton("onSingleton");
 
-  // Both `fireEvent` calls are dispatched asynchronously, so they won't
-  // have fired by this point. The `fail` listeners that we remove now
-  // should not be called, even though the events have already been
+  // The `fireSingleton` call ia dispatched asynchronously, so it won't
+  // have fired by this point. The `fail` listener that we remove now
+  // should not be called, even though the event has already been
   // enqueued.
-  onEvent.removeListener(fail);
   onSingleton.removeListener(fail);
 
-  // Wait for the remaining listeners to be called, which should always
-  // happen after the `fail` listeners would normally be called.
-  yield Promise.all(promises);
+  // Wait for the remaining listener to be called, which should always
+  // happen after the `fail` listener would normally be called.
+  yield promise;
 
-  // Check that event listeners aren't called after the context has
+  // Check that the event listener isn't called after the context has
   // unloaded.
-  onEvent.addListener(fail);
   onSingleton.addListener(fail);
 
-  // The EventManager `fire` callback always dispatches events
+  // The `fire` callback always dispatches events
   // asynchronously, so we need to test that any pending event callbacks
   // aren't fired after the context unloads. We also need to test that
   // any `fire` calls that happen *after* the context is unloaded also
   // do not trigger callbacks.
-  fireEvent("onEvent");
-  Promise.resolve("onEvent").then(fireEvent);
-
   fireSingleton("onSingleton");
   Promise.resolve("onSingleton").then(fireSingleton);
 
   context.unload();
 
   // The `setTimeout` ensures that we return to the event loop after
   // promise resolution, which means we're guaranteed to return after
   // any micro-tasks that get enqueued by the resolution handlers above.
--- a/toolkit/components/extensions/test/xpcshell/test_ext_idle.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_idle.js
@@ -140,16 +140,19 @@ add_task(function* testSetDetectionInter
   });
 
   idleService._reset();
   yield extension.startup();
   yield extension.awaitMessage("listenerAdded");
   idleService._fireObservers("idle");
   yield extension.awaitMessage("listenerFired");
   checkActivity({expectedAdd: [99], expectedRemove: [], expectedFires: ["idle"]});
+  // Defer unloading the extension so the asynchronous event listener
+  // reply finishes.
+  yield new Promise(resolve => setTimeout(resolve, 0));
   yield extension.unload();
 });
 
 add_task(function* testSetDetectionIntervalAfterAddingListener() {
   function background() {
     browser.idle.onStateChanged.addListener(newState => {
       browser.test.assertEq("idle", newState, "listener fired with the expected state");
       browser.test.sendMessage("listenerFired");
@@ -166,16 +169,20 @@ add_task(function* testSetDetectionInter
   });
 
   idleService._reset();
   yield extension.startup();
   yield extension.awaitMessage("detectionIntervalSet");
   idleService._fireObservers("idle");
   yield extension.awaitMessage("listenerFired");
   checkActivity({expectedAdd: [60, 99], expectedRemove: [60], expectedFires: ["idle"]});
+
+  // Defer unloading the extension so the asynchronous event listener
+  // reply finishes.
+  yield new Promise(resolve => setTimeout(resolve, 0));
   yield extension.unload();
 });
 
 add_task(function* testOnlyAddingListener() {
   function background() {
     browser.idle.onStateChanged.addListener(newState => {
       browser.test.assertEq("active", newState, "listener fired with the expected state");
       browser.test.sendMessage("listenerFired");
@@ -193,10 +200,14 @@ add_task(function* testOnlyAddingListene
   idleService._reset();
   yield extension.startup();
   yield extension.awaitMessage("listenerAdded");
   idleService._fireObservers("active");
   yield extension.awaitMessage("listenerFired");
   // check that "idle-daily" topic does not cause a listener to fire
   idleService._fireObservers("idle-daily");
   checkActivity({expectedAdd: [60], expectedRemove: [], expectedFires: ["active", "idle-daily"]});
+
+  // Defer unloading the extension so the asynchronous event listener
+  // reply finishes.
+  yield new Promise(resolve => setTimeout(resolve, 0));
   yield extension.unload();
 });