Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan draft
authorBob Silverberg <bsilverberg@mozilla.com>
Tue, 10 May 2016 12:14:44 -0400
changeset 365978 ddca9a13caf035d905d2a0f75a9a2dd48f804d35
parent 365944 8cf323be5c58b28d8719401ebb0ef63f1d71d000
child 520677 0f27a7c6cccbf9c42334c555dbff4ade162d78a2
push id17873
push userbmo:bob.silverberg@gmail.com
push dateWed, 11 May 2016 19:21:19 +0000
reviewersaswan
bugs1265797
milestone49.0a1
Bug 1265797 - WebExtensions Notification observer should not remove the notificationId on alertshow, r?aswan Note that I looked at trying to create a test to test this bug and which would prove it was fixed by the patch, but there is already a test in place [1] that would catch this if the timing was right. It looks like most of the time the call to clear() happens before the observer reacts to the `alertshow` topic and only if the opposite is true would the bug manifest itself. All of this is to say that I believe that the fix is a good one, but not one for which we can accurately test. [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_notifications.html#91 MozReview-Commit-ID: GLqHkHEnDwd
toolkit/components/extensions/ext-notifications.js
--- a/toolkit/components/extensions/ext-notifications.js
+++ b/toolkit/components/extensions/ext-notifications.js
@@ -49,29 +49,32 @@ Notification.prototype = {
       // This will fail if the OS doesn't support this function.
     }
     notificationsMap.get(this.extension).delete(this.id);
   },
 
   observe(subject, topic, data) {
     let notifications = notificationsMap.get(this.extension);
 
+    function emitAndDelete(event) {
+      notifications.emit(event, data);
+      notifications.delete(this.id);
+    }
+
     // Don't try to emit events if the extension has been unloaded
     if (!notifications) {
       return;
     }
 
     if (topic === "alertclickcallback") {
-      notifications.emit("clicked", data);
+      emitAndDelete("clicked");
     }
     if (topic === "alertfinished") {
-      notifications.emit("closed", data);
+      emitAndDelete("closed");
     }
-
-    notifications.delete(this.id);
   },
 };
 
 /* eslint-disable mozilla/balanced-listeners */
 extensions.on("startup", (type, extension) => {
   let map = new Map();
   EventEmitter.decorate(map);
   notificationsMap.set(extension, map);