Bug 1252303 - Creating an alarm should clear any existing alarms with the same name, r?kmag draft
authorbsilverberg <bsilverberg@mozilla.com>
Tue, 08 Mar 2016 14:03:41 -0500
changeset 338190 e43fcf71be9346becc512446fe511f87896b7955
parent 337431 5a8c334ac296c94faf804e12a05c3e5c6f555155
child 515756 7e1a418d7f479b291a036d9b785fe32d48abc210
push id12461
push userbmo:bob.silverberg@gmail.com
push dateTue, 08 Mar 2016 19:04:37 +0000
reviewerskmag
bugs1252303
milestone47.0a1
Bug 1252303 - Creating an alarm should clear any existing alarms with the same name, r?kmag I chose to test two conditions for this bug: 1. Check that getAll() returns the expected number of alarms (i.e., creating an alarm with the same name as an existing alarm does not create a new alarm). 2. Check that the original alarm, which is replaced with an alarm with a duplicate name, is in fact cleared and therefore does not fire. This also now includes: - change alarmsMap to use Maps instead of Sets - change get() to not set lastError when an alarm does not exist - rewrite tests to expect Promises from the alarms API MozReview-Commit-ID: JYvbTcNTKem
toolkit/components/extensions/ext-alarms.js
toolkit/components/extensions/test/mochitest/test_ext_alarms.html
--- a/toolkit/components/extensions/ext-alarms.js
+++ b/toolkit/components/extensions/ext-alarms.js
@@ -2,17 +2,17 @@
 
 var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 var {
   EventManager,
 } = ExtensionUtils;
 
-// WeakMap[Extension -> Set[Alarm]]
+// WeakMap[Extension -> Map[name -> Alarm]]
 var alarmsMap = new WeakMap();
 
 // WeakMap[Extension -> Set[callback]]
 var alarmCallbacksMap = new WeakMap();
 
 // Manages an alarm created by the extension (alarms API).
 function Alarm(extension, name, alarmInfo) {
   this.extension = extension;
@@ -39,17 +39,17 @@ function Alarm(extension, name, alarmInf
   let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
   timer.init(this, delay, Ci.nsITimer.TYPE_ONE_SHOT);
   this.timer = timer;
 }
 
 Alarm.prototype = {
   clear() {
     this.timer.cancel();
-    alarmsMap.get(this.extension).delete(this);
+    alarmsMap.get(this.extension).delete(this.name);
     this.canceled = true;
   },
 
   observe(subject, topic, data) {
     for (let callback of alarmCallbacksMap.get(this.extension)) {
       callback(this);
     }
     if (this.canceled) {
@@ -72,73 +72,69 @@ Alarm.prototype = {
       scheduledTime: this.scheduledTime,
       periodInMinutes: this.periodInMinutes,
     };
   },
 };
 
 /* eslint-disable mozilla/balanced-listeners */
 extensions.on("startup", (type, extension) => {
-  alarmsMap.set(extension, new Set());
+  alarmsMap.set(extension, new Map());
   alarmCallbacksMap.set(extension, new Set());
 });
 
 extensions.on("shutdown", (type, extension) => {
-  for (let alarm of alarmsMap.get(extension)) {
+  for (let alarm of alarmsMap.get(extension).values()) {
     alarm.clear();
   }
   alarmsMap.delete(extension);
   alarmCallbacksMap.delete(extension);
 });
 /* eslint-enable mozilla/balanced-listeners */
 
 extensions.registerSchemaAPI("alarms", "alarms", (extension, context) => {
   return {
     alarms: {
       create: function(name, alarmInfo) {
         name = name || "";
+        let alarms = alarmsMap.get(extension);
+        if (alarms.has(name)) {
+          alarms.get(name).clear();
+        }
         let alarm = new Alarm(extension, name, alarmInfo);
-        alarmsMap.get(extension).add(alarm);
+        alarms.set(alarm.name, alarm);
       },
 
       get: function(name) {
         name = name || "";
-        for (let alarm of alarmsMap.get(extension)) {
-          if (alarm.name === name) {
-            return Promise.resolve(alarm.data);
-          }
+        let alarms = alarmsMap.get(extension);
+        if (alarms.has(name)) {
+          return Promise.resolve(alarms.get(name).data);
         }
-        return Promise.reject({message: `Alarm ${name} not found.`});
+        return Promise.resolve();
       },
 
       getAll: function() {
-        let alarms = alarmsMap.get(extension);
-        let result = Array.from(alarms, alarm => alarm.data);
+        let result = Array.from(alarmsMap.get(extension).values(), alarm => alarm.data);
         return Promise.resolve(result);
       },
 
       clear: function(name) {
         name = name || "";
         let alarms = alarmsMap.get(extension);
-        let cleared = false;
-        for (let alarm of alarms) {
-          if (alarm.name == name) {
-            alarm.clear();
-            cleared = true;
-            break;
-          }
+        if (alarms.has(name)) {
+          alarms.get(name).clear();
+          return Promise.resolve(true);
         }
-
-        return Promise.resolve(cleared);
+        return Promise.resolve(false);
       },
 
       clearAll: function() {
-        let alarms = alarmsMap.get(extension);
         let cleared = false;
-        for (let alarm of alarms) {
+        for (let alarm of alarmsMap.get(extension).values()) {
           alarm.clear();
           cleared = true;
         }
         return Promise.resolve(cleared);
       },
 
       onAlarm: new EventManager(context, "alarms.onAlarm", fire => {
         let callback = alarm => {
--- a/toolkit/components/extensions/test/mochitest/test_ext_alarms.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_alarms.html
@@ -33,17 +33,17 @@ add_task(function* test_alarm_without_pe
 });
 
 
 add_task(function* test_alarm_fires() {
   function backgroundScript() {
     let ALARM_NAME = "test_ext_alarms";
 
     browser.alarms.onAlarm.addListener(alarm => {
-      browser.test.assertEq(alarm.name, ALARM_NAME, "alarm has the correct name");
+      browser.test.assertEq(ALARM_NAME, alarm.name, "alarm has the correct name");
       browser.test.notifyPass("alarms");
     });
     browser.alarms.create(ALARM_NAME, {delayInMinutes: 0.02});
     setTimeout(() => {
       browser.test.fail("alarm fired within expected time");
     }, 10000);
   }
 
@@ -63,17 +63,18 @@ add_task(function* test_alarm_fires() {
 add_task(function* test_cleared_alarm_does_not_fire() {
   function backgroundScript() {
     let ALARM_NAME = "test_ext_alarms";
 
     browser.alarms.onAlarm.addListener(alarm => {
       browser.test.fail("cleared alarm does not fire");
     });
     browser.alarms.create(ALARM_NAME, {when: Date.now() + 1000});
-    browser.alarms.clear(ALARM_NAME, wasCleared => {
+
+    browser.alarms.clear(ALARM_NAME).then(wasCleared => {
       browser.test.assertTrue(wasCleared, "alarm was cleared");
       setTimeout(() => {
         browser.test.notifyPass("alarms");
       }, 2000);
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
@@ -89,23 +90,23 @@ add_task(function* test_cleared_alarm_do
 });
 
 
 add_task(function* test_alarm_fires_with_when() {
   function backgroundScript() {
     let ALARM_NAME = "test_ext_alarms";
 
     browser.alarms.onAlarm.addListener(alarm => {
-      browser.test.assertEq(alarm.name, ALARM_NAME, "alarm has the expected name");
+      browser.test.assertEq(ALARM_NAME, alarm.name, "alarm has the expected name");
       browser.test.notifyPass("alarms");
     });
     browser.alarms.create(ALARM_NAME, {when: Date.now() + 1000});
     setTimeout(() => {
       browser.test.fail("alarm fired within expected time");
-      browser.alarms.clear(ALARM_NAME, (wasCleared) => {
+      browser.alarms.clear(ALARM_NAME).then(wasCleared => {
         browser.test.assertTrue(wasCleared, "alarm was cleared");
       });
     }, 10000);
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     background: `(${backgroundScript})()`,
     manifest: {
@@ -120,22 +121,22 @@ add_task(function* test_alarm_fires_with
 
 
 add_task(function* test_alarm_clear_non_matching_name() {
   function backgroundScript() {
     let ALARM_NAME = "test_ext_alarms";
 
     browser.alarms.create(ALARM_NAME, {when: Date.now() + 2000});
 
-    browser.alarms.clear(ALARM_NAME + "1", wasCleared => {
+    browser.alarms.clear(ALARM_NAME + "1").then(wasCleared => {
       browser.test.assertFalse(wasCleared, "alarm was not cleared");
-      browser.alarms.getAll(alarms => {
-        browser.test.assertEq(1, alarms.length, "alarm was not removed");
-        browser.test.notifyPass("alarms");
-      });
+      return browser.alarms.getAll();
+    }).then(alarms => {
+      browser.test.assertEq(1, alarms.length, "alarm was not removed");
+      browser.test.notifyPass("alarms");
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     background: `(${backgroundScript})()`,
     manifest: {
       permissions: ["alarms"],
     },
@@ -146,25 +147,25 @@ add_task(function* test_alarm_clear_non_
   yield extension.unload();
 });
 
 
 add_task(function* test_alarm_get_and_clear_single_argument() {
   function backgroundScript() {
     browser.alarms.create({when: Date.now() + 2000});
 
-    browser.alarms.get(alarm => {
+    browser.alarms.get().then(alarm => {
       browser.test.assertEq("", alarm.name, "expected alarm returned");
-      browser.alarms.clear(wasCleared => {
-        browser.test.assertTrue(wasCleared, "alarm was cleared");
-        browser.alarms.getAll(alarms => {
-          browser.test.assertEq(0, alarms.length, "alarm was removed");
-          browser.test.notifyPass("alarms");
-        });
-      });
+      return browser.alarms.clear();
+    }).then(wasCleared => {
+      browser.test.assertTrue(wasCleared, "alarm was cleared");
+      return browser.alarms.getAll();
+    }).then(alarms => {
+      browser.test.assertEq(0, alarms.length, "alarm was removed");
+      browser.test.notifyPass("alarms");
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     background: `(${backgroundScript})()`,
     manifest: {
       permissions: ["alarms"],
     },
@@ -179,26 +180,26 @@ add_task(function* test_alarm_get_and_cl
 add_task(function* test_periodic_alarm_fires() {
   function backgroundScript() {
     const ALARM_NAME = "test_ext_alarms";
 
     let count = 0;
     browser.alarms.onAlarm.addListener(alarm => {
       browser.test.assertEq(alarm.name, ALARM_NAME, "alarm has the expected name");
       if (count++ === 3) {
-        browser.alarms.clear(ALARM_NAME, (wasCleared) => {
+        browser.alarms.clear(ALARM_NAME).then(wasCleared => {
           browser.test.assertTrue(wasCleared, "alarm was cleared");
           browser.test.notifyPass("alarms");
         });
       }
     });
     browser.alarms.create(ALARM_NAME, {periodInMinutes: 0.02});
     setTimeout(() => {
       browser.test.notify("alarm fired within expected time");
-      browser.alarms.clear(ALARM_NAME, (wasCleared) => {
+      browser.alarms.clear(ALARM_NAME).then(wasCleared => {
         browser.test.assertTrue(wasCleared, "alarm was cleared");
       });
     }, 30000);
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     background: `(${backgroundScript})()`,
     manifest: {
@@ -213,62 +214,53 @@ add_task(function* test_periodic_alarm_f
 
 
 add_task(function* test_get_get_all_clear_all_alarms() {
   function backgroundScript() {
     const ALARM_NAME = "test_alarm";
 
     let suffixes = [0, 1, 2];
 
-    // Wrap API methods in promise-based variants.
-    let promiseAlarms = {};
-    Object.keys(browser.alarms).forEach(method => {
-      promiseAlarms[method] = (...args) => {
-        return new Promise(resolve => {
-          browser.alarms[method](...args, resolve);
-        });
-      };
-    });
+    for (let suffix of suffixes) {
+      browser.alarms.create(ALARM_NAME + suffix, {when: Date.now() + (suffix + 1) * 10000});
+    }
 
-    suffixes.forEach(suffix => {
-      browser.alarms.create(ALARM_NAME + suffix, {when: Date.now() + (suffix + 1) * 10000});
-    });
-
-    promiseAlarms.getAll().then(alarms => {
-      browser.test.assertEq(suffixes.length, alarms.length);
+    browser.alarms.getAll().then(alarms => {
+      browser.test.assertEq(suffixes.length, alarms.length, "expected number of alarms were found");
       alarms.forEach((alarm, index) => {
         browser.test.assertEq(ALARM_NAME + index, alarm.name, "alarm has the expected name");
       });
 
       return Promise.all(
         suffixes.map(suffix => {
-          return promiseAlarms.get(ALARM_NAME + suffix).then(alarm => {
+          return browser.alarms.get(ALARM_NAME + suffix).then(alarm => {
             browser.test.assertEq(ALARM_NAME + suffix, alarm.name, "alarm has the expected name");
             browser.test.sendMessage(`get-${suffix}`);
           });
-        }));
+        })
+      );
     }).then(() => {
-      return promiseAlarms.clear(ALARM_NAME + suffixes[0]);
+      return browser.alarms.clear(ALARM_NAME + suffixes[0]);
     }).then(wasCleared => {
       browser.test.assertTrue(wasCleared, "alarm was cleared");
 
-      return promiseAlarms.getAll();
+      return browser.alarms.getAll();
     }).then(alarms => {
       browser.test.assertEq(2, alarms.length, "alarm was removed");
 
-      return promiseAlarms.get(ALARM_NAME + suffixes[0]);
+      return browser.alarms.get(ALARM_NAME + suffixes[0]);
     }).then(alarm => {
       browser.test.assertEq(undefined, alarm, "non-existent alarm is undefined");
       browser.test.sendMessage(`get-invalid`);
 
-      return promiseAlarms.clearAll();
+      return browser.alarms.clearAll();
     }).then(wasCleared => {
       browser.test.assertTrue(wasCleared, "alarms were cleared");
 
-      return promiseAlarms.getAll();
+      return browser.alarms.getAll();
     }).then(alarms => {
       browser.test.assertEq(0, alarms.length, "no alarms exist");
       browser.test.sendMessage("clearAll");
       browser.test.sendMessage("clear");
       browser.test.sendMessage("getAll");
     });
   }
 
@@ -287,12 +279,53 @@ add_task(function* test_get_get_all_clea
     extension.awaitMessage("get-2"),
     extension.awaitMessage("clear"),
     extension.awaitMessage("get-invalid"),
     extension.awaitMessage("clearAll"),
   ]);
   yield extension.unload();
 });
 
+add_task(function* test_duplicate_alarm_name_replaces_alarm() {
+  function backgroundScript() {
+    let count = 0;
+
+    browser.alarms.onAlarm.addListener(alarm => {
+      if (alarm.name === "master alarm") {
+        browser.alarms.create("child alarm", {delayInMinutes: 0.05});
+        browser.alarms.getAll().then(results => {
+          browser.test.assertEq(2, results.length, "exactly two alarms exist");
+          browser.test.assertEq("master alarm", results[0].name, "first alarm has the expected name");
+          browser.test.assertEq("child alarm", results[1].name, "second alarm has the expected name");
+        }).then(() => {
+          if (count++ === 3) {
+            browser.alarms.clear("master alarm").then(wasCleared => {
+              return browser.alarms.clear("child alarm");
+            }).then(wasCleared => {
+              browser.test.notifyPass("alarms");
+            });
+          }
+        });
+      } else {
+        browser.test.fail("duplicate named alarm replaced existing alarm");
+        browser.test.notifyFail("alarms");
+      }
+    });
+
+    browser.alarms.create("master alarm", {delayInMinutes: 0.025, periodInMinutes: 0.025});
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    background: `(${backgroundScript})()`,
+    manifest: {
+      permissions: ["alarms"],
+    },
+  });
+
+  yield extension.startup();
+  yield extension.awaitFinish("alarms");
+  yield extension.unload();
+});
+
 </script>
 
 </body>
 </html>