Bug 1631011 - Fire compose.onBeforeSend sequentially instead of all at once. r=mkmelin a=wsmwk THUNDERBIRD_76_0b3_BUILD1 THUNDERBIRD_76_0b3_RELEASE
authorGeoff Lankow <geoff@darktrojan.net>
Fri, 17 Apr 2020 21:29:18 +1200
changeset 38721 9c50eec1c6cd684f60741ecf1100eb429bf0508a
parent 38720 9ef0d5ece977ce41a31434d4f4f45debed785a4c
child 38722 3728645af33c9580405f417af633659b77f1bc84
push id400
push userclokep@gmail.com
push dateMon, 04 May 2020 18:56:09 +0000
reviewersmkmelin, wsmwk
bugs1631011
Bug 1631011 - Fire compose.onBeforeSend sequentially instead of all at once. r=mkmelin a=wsmwk
mail/components/extensions/parent/ext-compose.js
mail/components/extensions/test/browser/browser_ext_compose_onBeforeSend.js
--- a/mail/components/extensions/parent/ext-compose.js
+++ b/mail/components/extensions/parent/ext-compose.js
@@ -206,82 +206,68 @@ async function setComposeDetails(compose
     }
   }
   if (Array.isArray(details.newsgroups)) {
     details.newsgroups = details.newsgroups.join(",");
   }
   composeWindow.SetComposeDetails(details);
 }
 
-class ComposeEventTracker extends EventEmitter {
-  constructor(extension) {
-    super();
-    this.extension = extension;
-    this.listenerCount = 0;
-  }
-  on(event, listener) {
-    super.on(event, listener);
+var composeEventTracker = {
+  listeners: new Set(),
 
-    this.listenerCount++;
-    if (this.listenerCount == 1) {
+  addListener(listener) {
+    this.listeners.add(listener);
+    if (this.listeners.size == 1) {
       windowTracker.addListener("beforesend", this);
     }
-  }
-  off(event, listener) {
-    super.off(event, listener);
-
-    this.listenerCount--;
-    if (this.listenerCount == 0) {
+  },
+  removeListener(listener) {
+    this.listeners.delete(listener);
+    if (this.listeners.size == 0) {
       windowTracker.removeListener("beforesend", this);
     }
-  }
+  },
   async handleEvent(event) {
     event.preventDefault();
 
     let msgType = event.detail;
     let composeWindow = event.target;
 
     composeWindow.ToggleWindowLock(true);
+    let didSetDetails = false;
 
-    let results = await this.emit(
-      "compose-before-send",
-      composeWindow,
-      getComposeDetails(composeWindow, this.extension)
-    );
-    let didSetDetails = false;
-    if (results) {
-      for (let result of results) {
-        if (!result) {
-          continue;
-        }
-        if (result.cancel) {
-          composeWindow.ToggleWindowLock(false);
-          return;
-        }
-        if (result.details) {
-          await setComposeDetails(
-            composeWindow,
-            result.details,
-            this.extension
-          );
-          didSetDetails = true;
-        }
+    for (let { handler, extension } of this.listeners) {
+      let result = await handler(
+        composeWindow,
+        getComposeDetails(composeWindow, extension)
+      );
+      if (!result) {
+        continue;
+      }
+      if (result.cancel) {
+        composeWindow.ToggleWindowLock(false);
+        return;
+      }
+      if (result.details) {
+        await setComposeDetails(composeWindow, result.details, extension);
+        didSetDetails = true;
       }
     }
 
     if (didSetDetails) {
       // Load the new details into gMsgCompose.compFields for sending.
       composeWindow.GetComposeDetails();
     }
     // Calling getComposeDetails collapses mailing lists. Expand them again.
     composeWindow.expandRecipients();
     composeWindow.ToggleWindowLock(false);
     composeWindow.CompleteGenericSendMessage(msgType);
-  }
-}
+  },
+};
 
 this.compose = class extends ExtensionAPI {
   getAPI(context) {
     function getComposeTab(tabId) {
       let tab = tabManager.get(tabId);
       if (tab instanceof TabmailTab) {
         throw new ExtensionError("Not a valid compose window");
       }
@@ -292,36 +278,38 @@ this.compose = class extends ExtensionAP
       ) {
         throw new ExtensionError(`Not a valid compose window: ${location}`);
       }
       return tab;
     }
 
     let { extension } = context;
     let { tabManager, windowManager } = extension;
-    this.eventTracker = new ComposeEventTracker(extension);
 
     return {
       compose: {
         onBeforeSend: new EventManager({
           context,
           name: "compose.onBeforeSend",
           inputHandling: true,
           register: fire => {
-            let listener = (event, window, details) => {
-              let win = windowManager.wrapWindow(window);
-              return fire.async(
-                tabManager.convert(win.activeTab.nativeTab),
-                details
-              );
+            let listener = {
+              handler(window, details) {
+                let win = windowManager.wrapWindow(window);
+                return fire.async(
+                  tabManager.convert(win.activeTab.nativeTab),
+                  details
+                );
+              },
+              extension,
             };
 
-            this.eventTracker.on("compose-before-send", listener);
+            composeEventTracker.addListener(listener);
             return () => {
-              this.eventTracker.off("compose-before-send", listener);
+              composeEventTracker.removeListener(listener);
             };
           },
         }).api(),
         beginNew(details) {
           return openComposeWindow(
             null,
             Ci.nsIMsgCompType.New,
             details,
--- a/mail/components/extensions/test/browser/browser_ext_compose_onBeforeSend.js
+++ b/mail/components/extensions/test/browser/browser_ext_compose_onBeforeSend.js
@@ -4,16 +4,17 @@
 
 var { ExtensionSupport } = ChromeUtils.import(
   "resource:///modules/ExtensionSupport.jsm"
 );
 
 let account = createAccount();
 let defaultIdentity = addIdentity(account);
 let nonDefaultIdentity = addIdentity(account, "nondefault@invalid");
+let outbox = account.incomingServer.rootFolder.getChildNamed("outbox");
 
 add_task(async function testCancel() {
   let extension = ExtensionTestUtils.loadExtension({
     background: async () => {
       function waitForEvent(eventName) {
         return new Promise(resolve => {
           let listener = window => {
             browser.windows[eventName].removeListener(listener);
@@ -79,27 +80,29 @@ add_task(async function testCancel() {
       let listener1 = tab => {
         listener1.tab = tab;
         return {};
       };
       browser.compose.onBeforeSend.addListener(listener1);
       await beginSend(true);
       browser.test.assertEq(tab.id, listener1.tab.id, "listener1 was fired");
       browser.compose.onBeforeSend.removeListener(listener1);
+      delete listener1.tab;
 
       // Add a cancelling listener. Sending should not continue.
 
       let listener2 = tab => {
         listener2.tab = tab;
         return { cancel: true };
       };
       browser.compose.onBeforeSend.addListener(listener2);
       await beginSend(false, false);
       browser.test.assertEq(tab.id, listener2.tab.id, "listener2 was fired");
       browser.compose.onBeforeSend.removeListener(listener2);
+      delete listener2.tab;
       await beginSend(true); // Removing the listener worked.
 
       // Add a listener returning a Promise. Resolve the Promise to unblock.
       // Sending should continue.
 
       let listener3 = tab => {
         listener3.tab = tab;
         return new Promise(resolve => {
@@ -107,40 +110,59 @@ add_task(async function testCancel() {
         });
       };
       browser.compose.onBeforeSend.addListener(listener3);
       await beginSend(false, true);
       browser.test.assertEq(tab.id, listener3.tab.id, "listener3 was fired");
       listener3.resolve({ cancel: false });
       await checkIfSent(true);
       browser.compose.onBeforeSend.removeListener(listener3);
+      delete listener3.tab;
 
       // Add a listener returning a Promise. Resolve the Promise to cancel.
       // Sending should not continue.
 
       let listener4 = tab => {
         listener4.tab = tab;
         return new Promise(resolve => {
           listener4.resolve = resolve;
         });
       };
       browser.compose.onBeforeSend.addListener(listener4);
       await beginSend(false, true);
       browser.test.assertEq(tab.id, listener4.tab.id, "listener4 was fired");
       listener4.resolve({ cancel: true });
       await checkIfSent(false, false);
       browser.compose.onBeforeSend.removeListener(listener4);
+      delete listener4.tab;
       await beginSend(true); // Removing the listener worked.
 
       // Clean up.
 
       let removedWindowPromise = waitForEvent("onRemoved");
       browser.windows.remove(createdWindow.id);
       await removedWindowPromise;
 
+      browser.test.assertTrue(
+        !listener1.tab,
+        "listener1 was not fired after removal"
+      );
+      browser.test.assertTrue(
+        !listener2.tab,
+        "listener2 was not fired after removal"
+      );
+      browser.test.assertTrue(
+        !listener3.tab,
+        "listener3 was not fired after removal"
+      );
+      browser.test.assertTrue(
+        !listener4.tab,
+        "listener4 was not fired after removal"
+      );
+
       browser.test.notifyPass("finished");
     },
     manifest: { permissions: ["compose"] },
   });
 
   // We can't allow sending to actually happen, this is a test. For every
   // compose window that opens, replace the function which does the actual
   // sending with one that only records when it has been called.
@@ -278,16 +300,17 @@ add_task(async function testChangeDetail
         "listener5 recipient correct"
       );
       browser.test.assertEq(
         "Test",
         listener5.details.subject,
         "listener5 subject correct"
       );
       browser.compose.onBeforeSend.removeListener(listener5);
+      delete listener5.tab;
 
       // Do the same thing, but this time with a Promise.
 
       createdWindowPromise = waitForEvent("onCreated");
       await browser.compose.beginNew({
         to: ["test@test.invalid"],
         subject: "Test",
         body: "Original body.",
@@ -330,16 +353,26 @@ add_task(async function testChangeDetail
           identityId: nonDefaultIdentity.id,
           to: ["to@test6.invalid"],
           cc: ["cc@test6.invalid"],
           subject: "Changed by listener6",
           body: "New body from listener6.",
         },
       });
       browser.compose.onBeforeSend.removeListener(listener6);
+      delete listener6.tab;
+
+      browser.test.assertTrue(
+        !listener5.tab,
+        "listener5 was not fired after removal"
+      );
+      browser.test.assertTrue(
+        !listener6.tab,
+        "listener6 was not fired after removal"
+      );
 
       browser.test.notifyPass("finished");
     },
     manifest: { permissions: ["accountsRead", "compose"] },
   });
 
   extension.onMessage("beginSend", async () => {
     let composeWindows = [...Services.wm.getEnumerator("msgcompose")];
@@ -358,20 +391,19 @@ add_task(async function testChangeDetail
 
     extension.sendMessage();
   });
 
   await extension.startup();
   await extension.awaitFinish("finished");
   await extension.unload();
 
-  let outbox = account.incomingServer.rootFolder.getChildNamed("outbox");
   let outboxMessages = outbox.messages;
   ok(outboxMessages.hasMoreElements());
-  let sentMessage5 = outboxMessages.getNext();
+  let sentMessage5 = outboxMessages.getNext().QueryInterface(Ci.nsIMsgDBHdr);
   is(sentMessage5.author, "nondefault@invalid", "author was changed");
   is(sentMessage5.subject, "Changed by listener5", "subject was changed");
   is(sentMessage5.recipients, "to@test5.invalid", "to was changed");
   is(sentMessage5.ccList, "cc@test5.invalid", "cc was changed");
 
   await new Promise(resolve => {
     window.MsgHdrToMimeMessage(sentMessage5, null, (msgHdr, mimeMessage) => {
       is(
@@ -379,17 +411,17 @@ add_task(async function testChangeDetail
         mimeMessage.parts[0].body.replace(/\r/g, ""),
         "New body from listener5.\n"
       );
       resolve();
     });
   });
 
   ok(outboxMessages.hasMoreElements());
-  let sentMessage6 = outboxMessages.getNext();
+  let sentMessage6 = outboxMessages.getNext().QueryInterface(Ci.nsIMsgDBHdr);
   is(sentMessage6.author, "nondefault@invalid", "author was changed");
   is(sentMessage6.subject, "Changed by listener6", "subject was changed");
   is(sentMessage6.recipients, "to@test6.invalid", "to was changed");
   is(sentMessage6.ccList, "cc@test6.invalid", "cc was changed");
 
   await new Promise(resolve => {
     window.MsgHdrToMimeMessage(sentMessage6, null, (msgHdr, mimeMessage) => {
       is(
@@ -568,34 +600,33 @@ add_task(async function testListExpansio
     await checkComposeHeaders(expected);
     extension.sendMessage();
   });
 
   await extension.startup();
   await extension.awaitFinish("finished");
   await extension.unload();
 
-  let outbox = account.incomingServer.rootFolder.getChildNamed("outbox");
   let outboxMessages = outbox.messages;
   ok(outboxMessages.hasMoreElements());
-  let sentMessage7 = outboxMessages.getNext();
+  let sentMessage7 = outboxMessages.getNext().QueryInterface(Ci.nsIMsgDBHdr);
   is(sentMessage7.subject, "Changed by listener7", "subject was changed");
   is(
     sentMessage7.recipients,
     "Sherlock Holmes <sherlock@bakerstreet.invalid>, John Watson <john@bakerstreet.invalid>",
     "list in unchanged field was expanded"
   );
   is(
     sentMessage7.bccList,
     "Sherlock Holmes <sherlock@bakerstreet.invalid>, John Watson <john@bakerstreet.invalid>",
     "list in changed field was expanded"
   );
 
   ok(outboxMessages.hasMoreElements());
-  let sentMessage8 = outboxMessages.getNext();
+  let sentMessage8 = outboxMessages.getNext().QueryInterface(Ci.nsIMsgDBHdr);
   is(sentMessage8.subject, "Test", "subject was not changed");
   is(
     sentMessage8.recipients,
     "Sherlock Holmes <sherlock@bakerstreet.invalid>, John Watson <john@bakerstreet.invalid>",
     "list in unchanged field was expanded"
   );
 
   ok(!outboxMessages.hasMoreElements());
@@ -606,8 +637,146 @@ add_task(async function testListExpansio
       null,
       true,
       false,
       { OnStopCopy: resolve },
       false
     );
   });
 });
+
+add_task(async function testMultipleListeners() {
+  let extensionA = ExtensionTestUtils.loadExtension({
+    background: async () => {
+      let listener9 = (tab, details) => {
+        browser.test.log("listener9 was fired");
+        browser.test.sendMessage("listener9", details);
+        browser.compose.onBeforeSend.removeListener(listener9);
+        return {
+          details: {
+            to: ["recipient2@invalid"],
+            subject: "Changed by listener9",
+          },
+        };
+      };
+      browser.compose.onBeforeSend.addListener(listener9);
+
+      await browser.compose.beginNew({
+        to: "recipient1@invalid",
+        subject: "Initial subject",
+      });
+      browser.test.sendMessage("ready");
+    },
+    manifest: { permissions: ["compose"] },
+  });
+
+  let extensionB = ExtensionTestUtils.loadExtension({
+    background: async () => {
+      let listener10 = (tab, details) => {
+        browser.test.log("listener10 was fired");
+        browser.test.sendMessage("listener10", details);
+        browser.compose.onBeforeSend.removeListener(listener10);
+        return {
+          details: {
+            to: ["recipient3@invalid"],
+            subject: "Changed by listener10",
+          },
+        };
+      };
+      browser.compose.onBeforeSend.addListener(listener10);
+
+      let listener11 = (tab, details) => {
+        browser.test.log("listener11 was fired");
+        browser.test.sendMessage("listener11", details);
+        browser.compose.onBeforeSend.removeListener(listener11);
+        return {
+          details: {
+            to: ["recipient4@invalid"],
+            subject: "Changed by listener11",
+          },
+        };
+      };
+      browser.compose.onBeforeSend.addListener(listener11);
+      browser.test.sendMessage("ready");
+    },
+    manifest: { permissions: ["compose"] },
+  });
+
+  await extensionA.startup();
+  await extensionB.startup();
+
+  await extensionA.awaitMessage("ready");
+  await extensionB.awaitMessage("ready");
+
+  let composeWindows = [...Services.wm.getEnumerator("msgcompose")];
+  Assert.equal(composeWindows.length, 1);
+  Assert.equal(composeWindows[0].document.readyState, "complete");
+  composeWindows[0].GenericSendMessage(Ci.nsIMsgCompDeliverMode.Later);
+
+  let listener9Details = await extensionA.awaitMessage("listener9");
+  Assert.equal(listener9Details.to.length, 1);
+  Assert.equal(
+    listener9Details.to[0],
+    "recipient1@invalid",
+    "listener9 recipient correct"
+  );
+  Assert.equal(
+    listener9Details.subject,
+    "Initial subject",
+    "listener9 subject correct"
+  );
+
+  let listener10Details = await extensionB.awaitMessage("listener10");
+  Assert.equal(listener10Details.to.length, 1);
+  Assert.equal(
+    listener10Details.to[0],
+    "recipient2@invalid",
+    "listener10 recipient correct"
+  );
+  Assert.equal(
+    listener10Details.subject,
+    "Changed by listener9",
+    "listener10 subject correct"
+  );
+
+  let listener11Details = await extensionB.awaitMessage("listener11");
+  Assert.equal(listener11Details.to.length, 1);
+  Assert.equal(
+    listener11Details.to[0],
+    "recipient3@invalid",
+    "listener11 recipient correct"
+  );
+  Assert.equal(
+    listener11Details.subject,
+    "Changed by listener10",
+    "listener11 subject correct"
+  );
+
+  await extensionA.unload();
+  await extensionB.unload();
+
+  let outboxMessages = outbox.messages;
+  Assert.ok(outboxMessages.hasMoreElements());
+  let sentMessage = outboxMessages.getNext().QueryInterface(Ci.nsIMsgDBHdr);
+  Assert.equal(
+    sentMessage.subject,
+    "Changed by listener11",
+    "subject was changed"
+  );
+  Assert.equal(
+    sentMessage.recipients,
+    "recipient4@invalid",
+    "recipient was changed"
+  );
+
+  Assert.ok(!outboxMessages.hasMoreElements());
+
+  await new Promise(resolve => {
+    outbox.deleteMessages(
+      toXPCOMArray([sentMessage], Ci.nsIMutableArray),
+      null,
+      true,
+      false,
+      { OnStopCopy: resolve },
+      false
+    );
+  });
+});